From c094e57b2f1e5549ce88e0540ff04ed3f71fa23d Mon Sep 17 00:00:00 2001 From: Randy Stauner Date: Fri, 21 Jul 2023 16:00:12 -0700 Subject: [PATCH] Remove custom permission error code and restore path in read error 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 d878622782f8abed46afcbff1385af075c115859 and read errors in 503e9d50805769e9fc5034ed175062810e8f8f54 --- ext/bootsnap/bootsnap.c | 25 +++++++++--- lib/bootsnap/compile_cache.rb | 10 ----- lib/bootsnap/compile_cache/iseq.rb | 2 - lib/bootsnap/compile_cache/json.rb | 16 +++----- lib/bootsnap/compile_cache/yaml.rb | 64 +++++++++++------------------- test/compile_cache/yaml_test.rb | 15 +++++++ test/compile_cache_test.rb | 16 +++++++- 7 files changed, 80 insertions(+), 68 deletions(-) diff --git a/ext/bootsnap/bootsnap.c b/ext/bootsnap/bootsnap.c index c32bc33..58c7b3b 100644 --- a/ext/bootsnap/bootsnap.c +++ b/ext/bootsnap/bootsnap.c @@ -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, ¤t_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); @@ -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 @@ -717,12 +722,18 @@ 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); @@ -730,7 +741,10 @@ bs_fetch(char * path, VALUE path_v, char * cache_path, VALUE handler, VALUE args /* 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); @@ -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; } } @@ -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: diff --git a/lib/bootsnap/compile_cache.rb b/lib/bootsnap/compile_cache.rb index ee2f47e..ff83457 100644 --- a/lib/bootsnap/compile_cache.rb +++ b/lib/bootsnap/compile_cache.rb @@ -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 @@ -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/) diff --git a/lib/bootsnap/compile_cache/iseq.rb b/lib/bootsnap/compile_cache/iseq.rb index 2fc4ee2..1b7607a 100644 --- a/lib/bootsnap/compile_cache/iseq.rb +++ b/lib/bootsnap/compile_cache/iseq.rb @@ -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}") diff --git a/lib/bootsnap/compile_cache/json.rb b/lib/bootsnap/compile_cache/json.rb index cb4f5dd..531d44b 100644 --- a/lib/bootsnap/compile_cache/json.rb +++ b/lib/bootsnap/compile_cache/json.rb @@ -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) diff --git a/lib/bootsnap/compile_cache/yaml.rb b/lib/bootsnap/compile_cache/yaml.rb index 7a3b871..1acfcd7 100644 --- a/lib/bootsnap/compile_cache/yaml.rb +++ b/lib/bootsnap/compile_cache/yaml.rb @@ -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) @@ -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) @@ -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) @@ -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) diff --git a/test/compile_cache/yaml_test.rb b/test/compile_cache/yaml_test.rb index 632b7e1..cdbd9ef 100644 --- a/test/compile_cache/yaml_test.rb +++ b/test/compile_cache/yaml_test.rb @@ -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) diff --git a/test/compile_cache_test.rb b/test/compile_cache_test.rb index 3dcf8d4..74148bc 100644 --- a/test/compile_cache_test.rb +++ b/test/compile_cache_test.rb @@ -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 @@ -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