Skip to content

Commit c094e57

Browse files
rwstaunerrwstauner
committed
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 d878622 and read errors in 503e9d5
1 parent 640c126 commit c094e57

File tree

7 files changed

+80
-68
lines changed

7 files changed

+80
-68
lines changed

‎ext/bootsnap/bootsnap.c

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -682,10 +682,14 @@ bs_fetch(char * path, VALUE path_v, char * cache_path, VALUE handler, VALUE args
682682
VALUE output_data; /* return data, e.g. ruby hash or loaded iseq */
683683

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

686687
/* Open the source file and generate a cache key for it */
687688
current_fd = open_current_file(path, &current_key, &errno_provenance);
688-
if (current_fd < 0) goto fail_errno;
689+
if (current_fd < 0) {
690+
exception_message = path_v;
691+
goto fail_errno;
692+
}
689693

690694
/* Open the cache key if it exists, and read its cache key in */
691695
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
695699
rb_funcall(rb_mBootsnap, instrumentation_method, 2, cache_fd == CACHE_MISS ? sym_miss : sym_stale, path_v);
696700
}
697701
} else if (cache_fd < 0) {
702+
exception_message = rb_str_new_cstr(cache_path);
698703
goto fail_errno;
699704
} else {
700705
/* True if the cache existed and no invalidating changes have occurred since
@@ -717,20 +722,29 @@ bs_fetch(char * path, VALUE path_v, char * cache_path, VALUE handler, VALUE args
717722
else if (res == CACHE_UNCOMPILABLE) {
718723
/* If fetch_cached_data returned `Uncompilable` we fallback to `input_to_output`
719724
This happens if we have say, an unsafe YAML cache, but try to load it in safe mode */
720-
if ((input_data = bs_read_contents(current_fd, current_key.size, &errno_provenance)) == Qfalse) goto fail_errno;
725+
if ((input_data = bs_read_contents(current_fd, current_key.size, &errno_provenance)) == Qfalse){
726+
exception_message = path_v;
727+
goto fail_errno;
728+
}
721729
bs_input_to_output(handler, args, input_data, &output_data, &exception_tag);
722730
if (exception_tag != 0) goto raise;
723731
goto succeed;
724732
} else if (res == CACHE_MISS || res == CACHE_STALE) valid_cache = 0;
725-
else if (res == ERROR_WITH_ERRNO) goto fail_errno;
733+
else if (res == ERROR_WITH_ERRNO){
734+
exception_message = rb_str_new_cstr(cache_path);
735+
goto fail_errno;
736+
}
726737
else if (!NIL_P(output_data)) goto succeed; /* fast-path, goal */
727738
}
728739
close(cache_fd);
729740
cache_fd = -1;
730741
/* Cache is stale, invalid, or missing. Regenerate and write it out. */
731742

732743
/* Read the contents of the source file into a buffer */
733-
if ((input_data = bs_read_contents(current_fd, current_key.size, &errno_provenance)) == Qfalse) goto fail_errno;
744+
if ((input_data = bs_read_contents(current_fd, current_key.size, &errno_provenance)) == Qfalse){
745+
exception_message = path_v;
746+
goto fail_errno;
747+
}
734748

735749
/* Try to compile the input_data using input_to_storage(input_data) */
736750
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
767781
* No point raising an error */
768782
if (errno != ENOENT) {
769783
errno_provenance = "bs_fetch:unlink";
784+
exception_message = rb_str_new_cstr(cache_path);
770785
goto fail_errno;
771786
}
772787
}
@@ -785,7 +800,7 @@ bs_fetch(char * path, VALUE path_v, char * cache_path, VALUE handler, VALUE args
785800
return output_data;
786801
fail_errno:
787802
CLEANUP;
788-
exception = rb_syserr_new(errno, errno_provenance);
803+
exception = rb_syserr_new_str(errno, exception_message);
789804
rb_exc_raise(exception);
790805
__builtin_unreachable();
791806
raise:

‎lib/bootsnap/compile_cache.rb

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ def UNCOMPILABLE.inspect
88
end
99

1010
Error = Class.new(StandardError)
11-
PermissionError = Class.new(Error)
1211

1312
def self.setup(cache_dir:, iseq:, yaml:, json:, readonly: false)
1413
if iseq
@@ -43,15 +42,6 @@ def self.setup(cache_dir:, iseq:, yaml:, json:, readonly: false)
4342
end
4443
end
4544

46-
def self.permission_error(path)
47-
cpath = Bootsnap::CompileCache::ISeq.cache_dir
48-
raise(
49-
PermissionError,
50-
"bootsnap doesn't have permission to write cache entries in '#{cpath}' " \
51-
"(or, less likely, doesn't have permission to read '#{path}')",
52-
)
53-
end
54-
5545
def self.supported?
5646
# only enable on 'ruby' (MRI), POSIX (darwin, linux, *bsd), Windows (RubyInstaller2) and >= 2.3.0
5747
%w[ruby truffleruby].include?(RUBY_ENGINE) && RUBY_PLATFORM.match?(/darwin|linux|bsd|mswin|mingw|cygwin/)

‎lib/bootsnap/compile_cache/iseq.rb

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,6 @@ def load_iseq(path)
8787
return nil if defined?(Coverage) && Bootsnap::CompileCache::Native.coverage_running?
8888

8989
Bootsnap::CompileCache::ISeq.fetch(path.to_s)
90-
rescue Errno::EACCES
91-
Bootsnap::CompileCache.permission_error(path)
9290
rescue RuntimeError => error
9391
if error.message =~ /unmatched platform/
9492
puts("unmatched platform for file #{path}")

‎lib/bootsnap/compile_cache/json.rb

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -74,16 +74,12 @@ def load_file(path, *args)
7474
return super unless (kwargs.keys - ::Bootsnap::CompileCache::JSON.supported_options).empty?
7575
end
7676

77-
begin
78-
::Bootsnap::CompileCache::Native.fetch(
79-
Bootsnap::CompileCache::JSON.cache_dir,
80-
File.realpath(path),
81-
::Bootsnap::CompileCache::JSON,
82-
kwargs,
83-
)
84-
rescue Errno::EACCES
85-
::Bootsnap::CompileCache.permission_error(path)
86-
end
77+
::Bootsnap::CompileCache::Native.fetch(
78+
Bootsnap::CompileCache::JSON.cache_dir,
79+
File.realpath(path),
80+
::Bootsnap::CompileCache::JSON,
81+
kwargs,
82+
)
8783
end
8884

8985
ruby2_keywords :load_file if respond_to?(:ruby2_keywords, true)

‎lib/bootsnap/compile_cache/yaml.rb

Lines changed: 24 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -229,16 +229,12 @@ def load_file(path, *args)
229229
return super unless (kwargs.keys - CompileCache::YAML.supported_options).empty?
230230
end
231231

232-
begin
233-
CompileCache::Native.fetch(
234-
CompileCache::YAML.cache_dir,
235-
File.realpath(path),
236-
CompileCache::YAML::Psych4::SafeLoad,
237-
kwargs,
238-
)
239-
rescue Errno::EACCES
240-
CompileCache.permission_error(path)
241-
end
232+
CompileCache::Native.fetch(
233+
CompileCache::YAML.cache_dir,
234+
File.realpath(path),
235+
CompileCache::YAML::Psych4::SafeLoad,
236+
kwargs,
237+
)
242238
end
243239

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

256-
begin
257-
CompileCache::Native.fetch(
258-
CompileCache::YAML.cache_dir,
259-
File.realpath(path),
260-
CompileCache::YAML::Psych4::UnsafeLoad,
261-
kwargs,
262-
)
263-
rescue Errno::EACCES
264-
CompileCache.permission_error(path)
265-
end
252+
CompileCache::Native.fetch(
253+
CompileCache::YAML.cache_dir,
254+
File.realpath(path),
255+
CompileCache::YAML::Psych4::UnsafeLoad,
256+
kwargs,
257+
)
266258
end
267259

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

312-
begin
313-
CompileCache::Native.fetch(
314-
CompileCache::YAML.cache_dir,
315-
File.realpath(path),
316-
CompileCache::YAML::Psych3,
317-
kwargs,
318-
)
319-
rescue Errno::EACCES
320-
CompileCache.permission_error(path)
321-
end
304+
CompileCache::Native.fetch(
305+
CompileCache::YAML.cache_dir,
306+
File.realpath(path),
307+
CompileCache::YAML::Psych3,
308+
kwargs,
309+
)
322310
end
323311

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

336-
begin
337-
CompileCache::Native.fetch(
338-
CompileCache::YAML.cache_dir,
339-
File.realpath(path),
340-
CompileCache::YAML::Psych3,
341-
kwargs,
342-
)
343-
rescue Errno::EACCES
344-
CompileCache.permission_error(path)
345-
end
324+
CompileCache::Native.fetch(
325+
CompileCache::YAML.cache_dir,
326+
File.realpath(path),
327+
CompileCache::YAML::Psych3,
328+
kwargs,
329+
)
346330
end
347331

348332
ruby2_keywords :unsafe_load_file if respond_to?(:ruby2_keywords, true)

‎test/compile_cache/yaml_test.rb

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,21 @@ def test_unsafe_load_file_supports_regexp
245245
end
246246
end
247247

248+
def test_no_read_permission
249+
if RbConfig::CONFIG["host_os"] =~ /mswin|mingw|cygwin/
250+
# On windows removing read permission doesn't prevent reading.
251+
pass
252+
else
253+
file = "a.yml"
254+
Help.set_file(file, ::YAML.dump(Object.new), 100)
255+
FileUtils.chmod(0o000, file)
256+
exception = assert_raises(Errno::EACCES) do
257+
FakeYaml.load_file(file)
258+
end
259+
assert_match(file, exception.message)
260+
end
261+
end
262+
248263
private
249264

250265
def with_default_encoding_internal(encoding)

‎test/compile_cache_test.rb

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ def test_no_write_permission_to_cache
3838
# list contents.
3939
#
4040
# Since we can't read directories on windows, this specific test doesn't
41-
# make sense. In addtion we test read-only files in
41+
# make sense. In addition we test read-only files in
4242
# `test_can_open_read_only_cache` so we are covered testing reading
4343
# read-only files.
4444
pass
@@ -51,6 +51,20 @@ def test_no_write_permission_to_cache
5151
end
5252
end
5353

54+
def test_no_read_permission
55+
if RbConfig::CONFIG["host_os"] =~ /mswin|mingw|cygwin/
56+
# On windows removing read permission doesn't prevent reading.
57+
pass
58+
else
59+
path = Help.set_file("a.rb", "a = a = 3", 100)
60+
FileUtils.chmod(0o000, path)
61+
exception = assert_raises(LoadError) do
62+
load(path)
63+
end
64+
assert_match(path, exception.message)
65+
end
66+
end
67+
5468
def test_can_open_read_only_cache
5569
path = Help.set_file("a.rb", "a = a = 3", 100)
5670
# Load once to create the cache file

0 commit comments

Comments
 (0)