From d6c2ab2c015c047270c05fff239b0d1de3c5c1a0 Mon Sep 17 00:00:00 2001 From: Yusuke Endoh Date: Tue, 26 Jul 2022 21:17:30 +0900 Subject: [PATCH 1/3] Fix the test permission of "test_rm_rf" The test was added for [Bug #6756]. The ticket insisted `FileUtils.rm_rf` should delete an empty directory even if its permission is 000. However, the test tried to delete a directory with permission 700. --- test/fileutils/test_fileutils.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fileutils/test_fileutils.rb b/test/fileutils/test_fileutils.rb index e1e2a82..9918072 100644 --- a/test/fileutils/test_fileutils.rb +++ b/test/fileutils/test_fileutils.rb @@ -1790,7 +1790,7 @@ 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' From ec5d3b84ea1e1d1b13c7dcb5bbe6cd5208c20a49 Mon Sep 17 00:00:00 2001 From: Yusuke Endoh Date: Tue, 26 Jul 2022 21:23:47 +0900 Subject: [PATCH 2/3] Narrow the scope of ensure The ensure in postorder_traverse was added for [Bug #6756]. The intention was to try to delete the parent directory if it failed to get the children. (It may be possible to delete the directory if it is empty.) However, the ensure region rescue'ed not only "failure to get children" but also "failure to delete each child". Thus, the following raised Errno::ENOTEMPTY, but we expect it to raise Errno::EACCES. ``` $ mkdir foo $ touch foo/bar $ chmod 555 foo $ ruby -rfileutils -e 'FileUtils.rm_rf("foo")' ``` This changeset narrows the ensure region so that it rescues only "failure to get children". --- lib/fileutils.rb | 12 ++++++++++-- test/fileutils/test_fileutils.rb | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/lib/fileutils.rb b/lib/fileutils.rb index 7eb66dd..5ddefed 100644 --- a/lib/fileutils.rb +++ b/lib/fileutils.rb @@ -2328,13 +2328,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 diff --git a/test/fileutils/test_fileutils.rb b/test/fileutils/test_fileutils.rb index 9918072..ccf2719 100644 --- a/test/fileutils/test_fileutils.rb +++ b/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 From fa65d676ece93a1380b9e6564efa4b4566c7a44b Mon Sep 17 00:00:00 2001 From: Yusuke Endoh Date: Tue, 26 Jul 2022 21:31:27 +0900 Subject: [PATCH 3/3] FileUtils.rm* methods swallows only Errno::ENOENT when force is true MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... instead of any StandardError. To behave like the standard `rm` command, it should only ignore exceptions about not existing files, not every exception. This should make debugging some errors easier, because the expectation is that `rm -rf` will succeed if and only if, all given files (previously existent or not) are removed. However, due to this exception swallowing, this is not always the case. From the `rm` man page > COMPATIBILITY > > The rm utility differs from historical implementations in that the -f > option only masks attempts to remove non-existent files instead of > masking a large variety of errors. Co-Authored-By: David Rodríguez --- lib/fileutils.rb | 17 +++++++++++------ test/fileutils/test_fileutils.rb | 20 ++++++++++++++++++++ 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/lib/fileutils.rb b/lib/fileutils.rb index 5ddefed..c14e1dc 100644 --- a/lib/fileutils.rb +++ b/lib/fileutils.rb @@ -1165,7 +1165,7 @@ def mv(src, dest, force: nil, noop: nil, verbose: nil, secure: nil) # # Keyword arguments: # - # - force: true - ignores raised exceptions of StandardError + # - force: true - ignores raised exceptions of Errno::ENOENT # and its descendants. # - noop: true - does not remove files; returns +nil+. # - verbose: true - prints an equivalent command: @@ -1248,7 +1248,7 @@ def rm_f(list, noop: nil, verbose: nil) # # Keyword arguments: # - # - force: true - ignores raised exceptions of StandardError + # - force: true - ignores raised exceptions of Errno::ENOENT # and its descendants. # - noop: true - does not remove entries; returns +nil+. # - secure: true - 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,7 +1441,7 @@ 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]. # @@ -1445,6 +1449,7 @@ 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]. # diff --git a/test/fileutils/test_fileutils.rb b/test/fileutils/test_fileutils.rb index ccf2719..993f820 100644 --- a/test/fileutils/test_fileutils.rb +++ b/test/fileutils/test_fileutils.rb @@ -1814,6 +1814,26 @@ def test_rm_rf 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