Skip to content

Commit

Permalink
Remove custom permission error code and restore path in read error
Browse files Browse the repository at this point in the history
Add a test to prove that if there is an error reading a file
the message contains the problematic path.

The custom permission error class and method were designed
to warn about errors with the cache path
but we stopped reporting write errors in
d878622
and read errors in
503e9d5
  • Loading branch information
rwstauner committed Jul 24, 2023
1 parent 640c126 commit c094e57
Show file tree
Hide file tree
Showing 7 changed files with 80 additions and 68 deletions.
25 changes: 20 additions & 5 deletions ext/bootsnap/bootsnap.c
Original file line number Diff line number Diff line change
Expand Up @@ -682,10 +682,14 @@ bs_fetch(char * path, VALUE path_v, char * cache_path, VALUE handler, VALUE args
VALUE output_data; /* return data, e.g. ruby hash or loaded iseq */

VALUE exception; /* ruby exception object to raise instead of returning */
VALUE exception_message; /* ruby exception string to use instead of errno_provenance */

/* Open the source file and generate a cache key for it */
current_fd = open_current_file(path, &current_key, &errno_provenance);
if (current_fd < 0) goto fail_errno;
if (current_fd < 0) {
exception_message = path_v;
goto fail_errno;
}

/* Open the cache key if it exists, and read its cache key in */
cache_fd = open_cache_file(cache_path, &cached_key, &errno_provenance);
Expand All @@ -695,6 +699,7 @@ bs_fetch(char * path, VALUE path_v, char * cache_path, VALUE handler, VALUE args
rb_funcall(rb_mBootsnap, instrumentation_method, 2, cache_fd == CACHE_MISS ? sym_miss : sym_stale, path_v);
}
} else if (cache_fd < 0) {
exception_message = rb_str_new_cstr(cache_path);
goto fail_errno;
} else {
/* True if the cache existed and no invalidating changes have occurred since
Expand All @@ -717,20 +722,29 @@ bs_fetch(char * path, VALUE path_v, char * cache_path, VALUE handler, VALUE args
else if (res == CACHE_UNCOMPILABLE) {
/* If fetch_cached_data returned `Uncompilable` we fallback to `input_to_output`
This happens if we have say, an unsafe YAML cache, but try to load it in safe mode */
if ((input_data = bs_read_contents(current_fd, current_key.size, &errno_provenance)) == Qfalse) goto fail_errno;
if ((input_data = bs_read_contents(current_fd, current_key.size, &errno_provenance)) == Qfalse){
exception_message = path_v;
goto fail_errno;
}
bs_input_to_output(handler, args, input_data, &output_data, &exception_tag);
if (exception_tag != 0) goto raise;
goto succeed;
} else if (res == CACHE_MISS || res == CACHE_STALE) valid_cache = 0;
else if (res == ERROR_WITH_ERRNO) goto fail_errno;
else if (res == ERROR_WITH_ERRNO){
exception_message = rb_str_new_cstr(cache_path);
goto fail_errno;
}
else if (!NIL_P(output_data)) goto succeed; /* fast-path, goal */
}
close(cache_fd);
cache_fd = -1;
/* Cache is stale, invalid, or missing. Regenerate and write it out. */

/* Read the contents of the source file into a buffer */
if ((input_data = bs_read_contents(current_fd, current_key.size, &errno_provenance)) == Qfalse) goto fail_errno;
if ((input_data = bs_read_contents(current_fd, current_key.size, &errno_provenance)) == Qfalse){
exception_message = path_v;
goto fail_errno;
}

/* Try to compile the input_data using input_to_storage(input_data) */
exception_tag = bs_input_to_storage(handler, args, input_data, path_v, &storage_data);
Expand Down Expand Up @@ -767,6 +781,7 @@ bs_fetch(char * path, VALUE path_v, char * cache_path, VALUE handler, VALUE args
* No point raising an error */
if (errno != ENOENT) {
errno_provenance = "bs_fetch:unlink";
exception_message = rb_str_new_cstr(cache_path);
goto fail_errno;
}
}
Expand All @@ -785,7 +800,7 @@ bs_fetch(char * path, VALUE path_v, char * cache_path, VALUE handler, VALUE args
return output_data;
fail_errno:
CLEANUP;
exception = rb_syserr_new(errno, errno_provenance);
exception = rb_syserr_new_str(errno, exception_message);
rb_exc_raise(exception);
__builtin_unreachable();
raise:
Expand Down
10 changes: 0 additions & 10 deletions lib/bootsnap/compile_cache.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ def UNCOMPILABLE.inspect
end

Error = Class.new(StandardError)
PermissionError = Class.new(Error)

def self.setup(cache_dir:, iseq:, yaml:, json:, readonly: false)
if iseq
Expand Down Expand Up @@ -43,15 +42,6 @@ def self.setup(cache_dir:, iseq:, yaml:, json:, readonly: false)
end
end

def self.permission_error(path)
cpath = Bootsnap::CompileCache::ISeq.cache_dir
raise(
PermissionError,
"bootsnap doesn't have permission to write cache entries in '#{cpath}' " \
"(or, less likely, doesn't have permission to read '#{path}')",
)
end

def self.supported?
# only enable on 'ruby' (MRI), POSIX (darwin, linux, *bsd), Windows (RubyInstaller2) and >= 2.3.0
%w[ruby truffleruby].include?(RUBY_ENGINE) && RUBY_PLATFORM.match?(/darwin|linux|bsd|mswin|mingw|cygwin/)
Expand Down
2 changes: 0 additions & 2 deletions lib/bootsnap/compile_cache/iseq.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,6 @@ def load_iseq(path)
return nil if defined?(Coverage) && Bootsnap::CompileCache::Native.coverage_running?

Bootsnap::CompileCache::ISeq.fetch(path.to_s)
rescue Errno::EACCES
Bootsnap::CompileCache.permission_error(path)
rescue RuntimeError => error
if error.message =~ /unmatched platform/
puts("unmatched platform for file #{path}")
Expand Down
16 changes: 6 additions & 10 deletions lib/bootsnap/compile_cache/json.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,16 +74,12 @@ def load_file(path, *args)
return super unless (kwargs.keys - ::Bootsnap::CompileCache::JSON.supported_options).empty?
end

begin
::Bootsnap::CompileCache::Native.fetch(
Bootsnap::CompileCache::JSON.cache_dir,
File.realpath(path),
::Bootsnap::CompileCache::JSON,
kwargs,
)
rescue Errno::EACCES
::Bootsnap::CompileCache.permission_error(path)
end
::Bootsnap::CompileCache::Native.fetch(
Bootsnap::CompileCache::JSON.cache_dir,
File.realpath(path),
::Bootsnap::CompileCache::JSON,
kwargs,
)
end

ruby2_keywords :load_file if respond_to?(:ruby2_keywords, true)
Expand Down
64 changes: 24 additions & 40 deletions lib/bootsnap/compile_cache/yaml.rb
Original file line number Diff line number Diff line change
Expand Up @@ -229,16 +229,12 @@ def load_file(path, *args)
return super unless (kwargs.keys - CompileCache::YAML.supported_options).empty?
end

begin
CompileCache::Native.fetch(
CompileCache::YAML.cache_dir,
File.realpath(path),
CompileCache::YAML::Psych4::SafeLoad,
kwargs,
)
rescue Errno::EACCES
CompileCache.permission_error(path)
end
CompileCache::Native.fetch(
CompileCache::YAML.cache_dir,
File.realpath(path),
CompileCache::YAML::Psych4::SafeLoad,
kwargs,
)
end

ruby2_keywords :load_file if respond_to?(:ruby2_keywords, true)
Expand All @@ -253,16 +249,12 @@ def unsafe_load_file(path, *args)
return super unless (kwargs.keys - CompileCache::YAML.supported_options).empty?
end

begin
CompileCache::Native.fetch(
CompileCache::YAML.cache_dir,
File.realpath(path),
CompileCache::YAML::Psych4::UnsafeLoad,
kwargs,
)
rescue Errno::EACCES
CompileCache.permission_error(path)
end
CompileCache::Native.fetch(
CompileCache::YAML.cache_dir,
File.realpath(path),
CompileCache::YAML::Psych4::UnsafeLoad,
kwargs,
)
end

ruby2_keywords :unsafe_load_file if respond_to?(:ruby2_keywords, true)
Expand Down Expand Up @@ -309,16 +301,12 @@ def load_file(path, *args)
return super unless (kwargs.keys - CompileCache::YAML.supported_options).empty?
end

begin
CompileCache::Native.fetch(
CompileCache::YAML.cache_dir,
File.realpath(path),
CompileCache::YAML::Psych3,
kwargs,
)
rescue Errno::EACCES
CompileCache.permission_error(path)
end
CompileCache::Native.fetch(
CompileCache::YAML.cache_dir,
File.realpath(path),
CompileCache::YAML::Psych3,
kwargs,
)
end

ruby2_keywords :load_file if respond_to?(:ruby2_keywords, true)
Expand All @@ -333,16 +321,12 @@ def unsafe_load_file(path, *args)
return super unless (kwargs.keys - CompileCache::YAML.supported_options).empty?
end

begin
CompileCache::Native.fetch(
CompileCache::YAML.cache_dir,
File.realpath(path),
CompileCache::YAML::Psych3,
kwargs,
)
rescue Errno::EACCES
CompileCache.permission_error(path)
end
CompileCache::Native.fetch(
CompileCache::YAML.cache_dir,
File.realpath(path),
CompileCache::YAML::Psych3,
kwargs,
)
end

ruby2_keywords :unsafe_load_file if respond_to?(:ruby2_keywords, true)
Expand Down
15 changes: 15 additions & 0 deletions test/compile_cache/yaml_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,21 @@ def test_unsafe_load_file_supports_regexp
end
end

def test_no_read_permission
if RbConfig::CONFIG["host_os"] =~ /mswin|mingw|cygwin/
# On windows removing read permission doesn't prevent reading.
pass
else
file = "a.yml"
Help.set_file(file, ::YAML.dump(Object.new), 100)
FileUtils.chmod(0o000, file)
exception = assert_raises(Errno::EACCES) do
FakeYaml.load_file(file)
end
assert_match(file, exception.message)
end
end

private

def with_default_encoding_internal(encoding)
Expand Down
16 changes: 15 additions & 1 deletion test/compile_cache_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def test_no_write_permission_to_cache
# list contents.
#
# Since we can't read directories on windows, this specific test doesn't
# make sense. In addtion we test read-only files in
# make sense. In addition we test read-only files in
# `test_can_open_read_only_cache` so we are covered testing reading
# read-only files.
pass
Expand All @@ -51,6 +51,20 @@ def test_no_write_permission_to_cache
end
end

def test_no_read_permission
if RbConfig::CONFIG["host_os"] =~ /mswin|mingw|cygwin/
# On windows removing read permission doesn't prevent reading.
pass
else
path = Help.set_file("a.rb", "a = a = 3", 100)
FileUtils.chmod(0o000, path)
exception = assert_raises(LoadError) do
load(path)
end
assert_match(path, exception.message)
end
end

def test_can_open_read_only_cache
path = Help.set_file("a.rb", "a = a = 3", 100)
# Load once to create the cache file
Expand Down

0 comments on commit c094e57

Please sign in to comment.