Skip to content

Fix FileUtils.rm_rf to only mask exceptions about non existent files #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
deivid-rodriguez wants to merge 2 commits into from
Closed

Fix FileUtils.rm_rf to only mask exceptions about non existent files #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2e6762
Fix incorrect permissions for test
deivid-rodriguez Sep 1, 2021
1fb73bf
Fix `force` flag to `rm`
deivid-rodriguez Mar 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 29 additions & 23 deletions lib/fileutils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,8 +680,11 @@ def remove_entry_secure(path, force = false)
return
end
fullpath = File.expand_path(path)
st = File.lstat(fullpath)
unless st.directory?
ent = Entry_.new(fullpath)
if force && !ent.exist?
return
end
unless ent.directory?
File.unlink fullpath
return
end
Expand All @@ -700,7 +703,7 @@ def remove_entry_secure(path, force = false)
dot_file = fullpath + "/."
begin
File.open(dot_file) {|f|
unless fu_stat_identical_entry?(st, f.stat)
unless fu_stat_identical_entry?(ent, f.stat)
# symlink (TOC-to-TOU attack?)
File.unlink fullpath
return
Expand All @@ -710,7 +713,7 @@ def remove_entry_secure(path, force = false)
}
rescue Errno::EISDIR # JRuby in non-native mode can't open files as dirs
File.lstat(dot_file).tap {|fstat|
unless fu_stat_identical_entry?(st, fstat)
unless fu_stat_identical_entry?(ent, fstat)
# symlink (TOC-to-TOU attack?)
File.unlink fullpath
return
Expand All @@ -720,7 +723,7 @@ def remove_entry_secure(path, force = false)
}
end

unless fu_stat_identical_entry?(st, File.lstat(fullpath))
unless fu_stat_identical_entry?(ent, File.lstat(fullpath))
# TOC-to-TOU attack?
File.unlink fullpath
return
Expand All @@ -735,14 +738,10 @@ def remove_entry_secure(path, force = false)
end
end
root.postorder_traverse do |ent|
begin
ent.remove
rescue
raise unless force
end
next if force && !ent.exist?

ent.remove
end
rescue
raise unless force
end
module_function :remove_entry_secure

Expand All @@ -769,14 +768,10 @@ def fu_stat_identical_entry?(a, b) #:nodoc:
#
def remove_entry(path, force = false)
Entry_.new(path).postorder_traverse do |ent|
begin
ent.remove
rescue
raise unless force
end
next if force && !ent.exist?

ent.remove
end
rescue
raise unless force
end
module_function :remove_entry

Expand All @@ -785,9 +780,10 @@ def remove_entry(path, force = false)
# This method ignores StandardError if +force+ is true.
#
def remove_file(path, force = false)
Entry_.new(path).remove_file
rescue
raise unless force
ent = Entry_.new(path)
return if force && !ent.exist?

ent.remove_file
end
module_function :remove_file

Expand Down Expand Up @@ -1256,6 +1252,16 @@ def blockdev?
s and s.blockdev?
end

def dev
s = lstat!
s and s.dev
end

def ino
s = lstat!
s and s.ino
end

def socket?
s = lstat!
s and s.socket?
Expand Down Expand Up @@ -1485,7 +1491,7 @@ def postorder_traverse
end
end
end
ensure
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized this ensure might have been intentional.

The current behavior is, even if an exception happen, still try to remove the given folder (and most likely get a Errno::ENOTEMPTY exception, because the original exception probably prevented some of the files inside to be removed). In that case, the original exception is still reported as the exception cause.

In Bundler, I was not seeing the original error because we're failing to report the cause if it exists in a couple of places, but I will fix this in Bundler.

This is all to say that, if the behavior I proposed is to be implemented, we might still want to restore this ensure.


yield self
end

Expand Down
22 changes: 21 additions & 1 deletion test/fileutils/test_fileutils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1790,12 +1790,32 @@ def test_rm_rf
return if /mswin|mingw/ =~ RUBY_PLATFORM

mkdir 'tmpdatadir'
chmod 700, 'tmpdatadir'
chmod 0700, '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

Expand Down