Merge pull request #99 from mame/dont-swallow-standarderror-in-fileutils-rm_rf

FileUtils.rm_rf should ignore only Errno::ENOENT, not StandardError

Author: mame

lib/fileutils.rb

@@ -1165,7 +1165,7 @@ def mv(src, dest, force: nil, noop: nil, verbose: nil, secure: nil)
   #
   # Keyword arguments:
   #
-  # - <tt>force: true</tt> - ignores raised exceptions of StandardError
+  # - <tt>force: true</tt> - ignores raised exceptions of Errno::ENOENT
   #   and its descendants.
   # - <tt>noop: true</tt> - does not remove files; returns +nil+.
   # - <tt>verbose: true</tt> - prints an equivalent command:
@@ -1248,7 +1248,7 @@ def rm_f(list, noop: nil, verbose: nil)
   #
   # Keyword arguments:
   #
-  # - <tt>force: true</tt> - ignores raised exceptions of StandardError
+  # - <tt>force: true</tt> - ignores raised exceptions of Errno::ENOENT
   #   and its descendants.
   # - <tt>noop: true</tt> - does not remove entries; returns +nil+.
   # - <tt>secure: true</tt> - removes +src+ securely;
@@ -1315,7 +1315,7 @@ def rm_rf(list, noop: nil, verbose: nil, secure: nil)
   # see {Avoiding the TOCTTOU Vulnerability}[rdoc-ref:FileUtils@Avoiding+the+TOCTTOU+Vulnerability].
   #
   # Optional argument +force+ specifies whether to ignore
-  # raised exceptions of StandardError and its descendants.
+  # raised exceptions of Errno::ENOENT and its descendants.
   #
   # Related: {methods for deleting}[rdoc-ref:FileUtils@Deleting].
   #
@@ -1384,10 +1384,12 @@ def remove_entry_secure(path, force = false)
         ent.remove
       rescue
         raise unless force
+        raise unless Errno::ENOENT === $!
       end
     end
   rescue
     raise unless force
+    raise unless Errno::ENOENT === $!
   end
   module_function :remove_entry_secure
 
@@ -1413,7 +1415,7 @@ def fu_stat_identical_entry?(a, b)   #:nodoc:
   # should be {interpretable as a path}[rdoc-ref:FileUtils@Path+Arguments].
   #
   # Optional argument +force+ specifies whether to ignore
-  # raised exceptions of StandardError and its descendants.
+  # raised exceptions of Errno::ENOENT and its descendants.
   #
   # Related: FileUtils.remove_entry_secure.
   #
@@ -1423,10 +1425,12 @@ def remove_entry(path, force = false)
         ent.remove
       rescue
         raise unless force
+        raise unless Errno::ENOENT === $!
       end
     end
   rescue
     raise unless force
+    raise unless Errno::ENOENT === $!
   end
   module_function :remove_entry
 
@@ -1437,14 +1441,15 @@ def remove_entry(path, force = false)
   # should be {interpretable as a path}[rdoc-ref:FileUtils@Path+Arguments].
   #
   # Optional argument +force+ specifies whether to ignore
-  # raised exceptions of StandardError and its descendants.
+  # raised exceptions of Errno::ENOENT and its descendants.
   #
   # Related: {methods for deleting}[rdoc-ref:FileUtils@Deleting].
   #
   def remove_file(path, force = false)
     Entry_.new(path).remove_file
   rescue
     raise unless force
+    raise unless Errno::ENOENT === $!
   end
   module_function :remove_file
 
@@ -1456,7 +1461,7 @@ def remove_file(path, force = false)
   # should be {interpretable as a path}[rdoc-ref:FileUtils@Path+Arguments].
   #
   # Optional argument +force+ specifies whether to ignore
-  # raised exceptions of StandardError and its descendants.
+  # raised exceptions of Errno::ENOENT and its descendants.
   #
   # Related: {methods for deleting}[rdoc-ref:FileUtils@Deleting].
   #
@@ -2328,13 +2333,21 @@ def preorder_traverse
 
     def postorder_traverse
       if directory?
-        entries().each do |ent|
+        begin
+          children = entries()
+        rescue Errno::EACCES
+          # Failed to get the list of children.
+          # Assuming there is no children, try to process the parent directory.
+          yield self
+          return
+        end
+
+        children.each do |ent|
           ent.postorder_traverse do |e|
             yield e
           end
         end
       end
-    ensure
       yield self
     end
 

test/fileutils/test_fileutils.rb

@@ -750,6 +750,24 @@ def test_rm_r_pathname
     assert_file_not_exist 'tmp/tmpdir3'
   end
 
+  def test_rm_r_no_permissions
+    check_singleton :rm_rf
+
+    return if /mswin|mingw/ =~ RUBY_PLATFORM
+
+    mkdir 'tmpdatadir'
+    touch 'tmpdatadir/tmpdata'
+    chmod "-x", 'tmpdatadir'
+
+    begin
+      assert_raise Errno::EACCES do
+        rm_r 'tmpdatadir'
+      end
+    ensure
+      chmod "+x", 'tmpdatadir'
+    end
+  end
+
   def test_remove_entry_cjk_path
     dir = "tmpdir\u3042"
     my_rm_rf dir
@@ -1798,12 +1816,32 @@ def test_rm_rf
     return if /mswin|mingw/ =~ RUBY_PLATFORM
 
     mkdir 'tmpdatadir'
-    chmod 0o700, 'tmpdatadir'
+    chmod 0o000, 'tmpdatadir'
     rm_rf 'tmpdatadir'
 
     assert_file_not_exist 'tmpdatadir'
   end
 
+  def test_rm_rf_no_permissions
+    check_singleton :rm_rf
+
+    return if /mswin|mingw/ =~ RUBY_PLATFORM
+
+    mkdir 'tmpdatadir'
+    touch 'tmpdatadir/tmpdata'
+    chmod "-x", 'tmpdatadir'
+
+    begin
+      assert_raise Errno::EACCES do
+        rm_rf 'tmpdatadir'
+      end
+
+      assert_file_exist 'tmpdatadir'
+    ensure
+      chmod "+x", 'tmpdatadir'
+    end
+  end
+
   def test_rmdir
     check_singleton :rmdir