Merge pull request #451 from Shopify/rwstauner/permission-error

casperisfine · web-flow · commit 2ea29bed2c02 · 2023-07-25T08:37:51.000+02:00
Remove custom permission error code and restore path in read error
diff --git 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, &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);
@@ -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,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);
@@ -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
@@ -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) and TruffleRuby for POSIX (darwin, linux, *bsd), Windows (RubyInstaller2)
       %w[ruby truffleruby].include?(RUBY_ENGINE) &&
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
