Skip to content

Commit

Permalink
Merge pull request Shopify#451 from Shopify/rwstauner/permission-error
Browse files Browse the repository at this point in the history 
Remove custom permission error code and restore path in read error
  • Loading branch information
@casperisfine
casperisfine authored Jul 25, 2023
2 parents 34dba5c + c094e57 commit 2ea29be
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
View file
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
View file
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) and TruffleRuby for POSIX (darwin, linux, *bsd), Windows (RubyInstaller2)
%w[ruby truffleruby].include?(RUBY_ENGINE) &&
Expand Down
2 changes: 0 additions & 2 deletions lib/bootsnap/compile_cache/iseq.rb
View file
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
View file
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
View file
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
View file
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
View file
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 2ea29be

Please sign in to comment.