Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FIX] Sqlite3 Should create db for missing parent directories as well #51628

Merged
merged 1 commit into from May 6, 2024
+9 −11
Merged

[FIX] Sqlite3 Should create db for missing parent directories as well #51628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 3 additions & 7 deletions activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
View file
Open in desktop
Expand Up @@ -112,13 +112,9 @@ def initialize(...)
dirname = File.dirname(@config[:database])
unless File.directory?(dirname)
begin
Dir.mkdir(dirname)
rescue Errno::ENOENT => error
if error.message.include?("No such file or directory")
raise ActiveRecord::NoDatabaseError.new(connection_pool: @pool)
else
raise
end
FileUtils.mkdir_p(dirname)
rescue SystemCallError
raise ActiveRecord::NoDatabaseError.new(connection_pool: @pool)
end
end
end
Expand Down
10 changes: 6 additions & 4 deletions activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb
View file
Open in desktop
Expand Up @@ -23,12 +23,14 @@ def setup
)
end

def test_bad_connection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test shouldn't be deleted but updated so it still simulate a failure. Of course it's trickier now that mkdir_p is used.

One way could be to create a temporary directory and change its permissions. e.g.

dir = Dir.mktmpdir
File.chmod(0x000, dir)
db_path  = File.join(dir, "db/cant-be-created.db")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@byroot, observe the alterations made:

Before:

begin
  Dir.mkdir(dirname)
rescue Errno::ENOENT => error
  if error.message.include?("No such file or directory")
    raise ActiveRecord::NoDatabaseError.new(connection_pool: @pool)
  else
    raise
  end
end

Now:

  FileUtils.mkdir_p(dirname)

The removal of the rescue block is notable. Why?

  1. The likelihood of encountering an Errno::ENOENT exception diminishes since the file or directory will now be created.
  2. In scenarios where permission constraints prevent directory or file creation, triggering a failure, an Errno::EACCES exception will be raised instead.

Incorporating a failure test, I've implemented a test here activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb:16

def test_database_should_not_get_created_for_parent_directories_with_missing_permission
  dir = Dir.mktmpdir
  File.chmod(0x000, dir)
  db_path = File.join(dir, "db/cant-be-created.db")
  assert_raise Errno::EACCES do
    connection = SQLite3Adapter.new(adapter: "sqlite3", database: db_path)
    connection.drop_table "ex", if_exists: true
  end
end

This test is green on my local machine. And it's strange that in buildkite/rails CI it's failing. Could you please check here what I am doing wrong?

bin/test test/cases/adapters/sqlite3/sqlite3_adapter_test.rb
Using sqlite3
Run options: --seed 52964

# Running:

..................................................................................

Finished in 0.070632s, 1160.9469 runs/s, 2718.3146 assertions/s.
82 runs, 192 assertions, 0 failures, 0 errors, 0 skips

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, try File.chmod(0x000, dir) (not FileUtils).

Also the bubbled exception shouldn't be an errno, we should handle all SyscallError and wrap them in ActiveRecord::NoDatabaseError.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @byroot! I also attempted to use File.chmod(0x000, dir).
https://github.com/rails/rails/pull/51628/files#diff-91ca39a63c4ff81c0580c12472a0a709c1d9c428a3ace4970ff3076d90ba6b13

But that doesn't seems to work.
Interestingly, the same test runs fine on my local machine.

I debugged and discovered something unexpected in CI.

image

https://buildkite.com/rails/rails/builds/106656#018f1f16-c702-468a-9f0b-d14d5eeba392

That parent directory permission mode is: /tmp/d20240427-17-857wvl: 40000

And this parent directory is still able to make the child directorydb with /tmp/d20240427-17-857wvl/db: 40755

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@byroot , I've identified the issue above and why it's exhibiting this unusual behavior.

To replicate the behavior, I conducted independent experiments with the FileUtils.mkdir_p method. Consequently, I crafted a script to test the method:

require 'test/unit'
require 'fileutils'
require 'tmpdir'
require 'debug'

class TestFileUtils < Test::Unit::TestCase
  def test_write_independent_test_to_check_fileutils
    Dir.mktmpdir do |dir|
      # Create a directory with restricted permissions
      File.chmod(0o000, dir)

      # Attempt to create a directory within the restricted directory
      db_path = File.join(dir, "check-fileutils/db/-cinco-dog.sqlite3")

      assert_raise Errno::EACCES do
        FileUtils.mkdir_p(db_path)
      end
    end
  end
end

When I executed the above script on my local machine with Ruby installed (ruby 3.2.2 (2023-03-30 revision e51014f9c0) [arm64-darwin22]), it ran without issues:

(base)  manishsharma@Manishs-MacBook-Air  ~/Development/personal/opensource/experiment  ruby reproduce.rb
Loaded suite reproduce
Started
.
Finished in 0.000621 seconds.
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
1 tests, 1 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
1610.31 tests/s, 1610.31 assertions/s

Since Builtkite Rails CI runs on Docker, I decided to run the same script within a Docker container. For this purpose, I created the following Dockerfile:

FROM ruby:latest
COPY reproduce.rb .
ENTRYPOINT ["ruby", "reproduce.rb"]

The reproduce.rb script remains unchanged.

Here's the output from running and building the image:

(base)  manishsharma@Manishs-MacBook-Air  ~/Development/personal/opensource/experiment  docker run fileutils-reproduce
Loaded suite reproduce
Started
F
===============================================================================
Failure: test_write_independent_test_to_check_fileutils(TestFileUtils): <Errno::EACCES> exception was expected but none was thrown.
reproduce.rb:15:in `block in test_write_independent_test_to_check_fileutils'
     12:       # Attempt to create a directory within the restricted directory
     13:       db_path = File.join(dir, "check-fileutils/db/-cinco-dog.sqlite3")
     14:
  => 15:       assert_raise Errno::EACCES  do
     16:         FileUtils.mkdir_p(db_path)
     17:       end
     18:     end
/usr/local/lib/ruby/3.3.0/tmpdir.rb:99:in `mktmpdir'
reproduce.rb:8:in `test_write_independent_test_to_check_fileutils'
===============================================================================
Finished in 0.003941125 seconds.
-------------------------------------------------------------------------------
1 tests, 1 assertions, 1 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
0% passed
-------------------------------------------------------------------------------
253.73 tests/s, 253.73 assertions/s

The question arises: Why does the Docker environment not raise an exception?

Upon inspecting the Docker container, I discovered that chmod sets the correct permissions for the directory:

# pwd
/tmp
# ls -la
total 16
drwxrwxrwt 1 root root 4096 Apr 27 15:07 .
drwxr-xr-x 1 root root 4096 Apr 27 15:03 ..
d--------- 2 root root 4096 Apr 27 15:03 d20240427-1-ihvptr
d--------- 3 root root 4096 Apr 27 15:09 d20240427-24-sa4xz6

Creating another directory inside d20240427-24-sa4xz6 occurs without errors because both the owner and group have root privileges:

# cd d20240427-24-sa4xz6
# mkdir -p create-without-issue
# ls -la
total 16
d--------- 4 root root 4096 Apr 27 15:27 .
drwxrwxrwt 1 root root 4096 Apr 27 15:07 ..
drwxr-xr-x 2 root root 4096 Apr 27 15:27 create-without-issue
drwxr-xr-x 2 root root 4096 Apr 27 15:09 temp

In summary, the root user can create files in a directory marked as Read Only.

Confirmed the user in Builkite Rails CI pipeline as well it's a root user:

image

https://buildkite.com/rails/rails/builds/106668#018f2402-28f6-4a89-a27d-04b675b70ed6

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@byroot , did you have a chance to look at it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried solution:

  1. Make directories immutable (using chattr or chflags)

with chattr command it gives an error:
https://buildkite.com/rails/rails/builds/106846#018f48ea-55a2-4af2-9f8d-acf4c38f1a57

chattr: Operation not permitted while setting flags on <directory-name>

Outcome: Docker Deny access to some filesystem operations, like creating new device nodes, changing the owner of files, or altering attributes (including the immutable flag).

It's mentioned here: https://docs.docker.com/engine/security/#linux-kernel-capabilities

Solution to solve this is: This is related to capabilities thing: chattr requires CAPLINUX IMMUTABLE which is disabled in docker by default. Just add -- cap-add
LINUX_ IMMUTABLE to docker container start options to enable it.

That's itself another issue on. I don't know whether to proceed with this or not as we have to do modification in CI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, got side tracked, looking at it now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I think that the issue is CI runs as root, so it's hard to simulate any problem into creating a directory or file.

So while I don't like not covering this sort of things with a test, I don't quite see what we could do, and anyway it could likely fail in dozens of different ways, so coverage here will never be that good.

I'll remove the test, sorry for sending you on a goose chase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! @byroot. Goose chase helped me go dig deep into docker and linux.

error = assert_raise ActiveRecord::NoDatabaseError do
connection = SQLite3Adapter.new(adapter: "sqlite3", database: "/tmp/should/_not/_exist/-cinco-dog.db")
def test_database_should_get_created_when_missing_parent_directories_for_database_path
dir = Dir.mktmpdir
db_path = File.join(dir, "_not_exist/-cinco-dog.sqlite3")
assert_nothing_raised do
connection = SQLite3Adapter.new(adapter: "sqlite3", database: db_path)
connection.drop_table "ex", if_exists: true
end
assert_kind_of ActiveRecord::ConnectionAdapters::NullPool, error.connection_pool
assert SQLite3Adapter.database_exists?(adapter: "sqlite3", database: db_path)
end

def test_database_exists_returns_false_when_the_database_does_not_exist
Expand Down