Skip to content
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.

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

rm: fix issue #6620 (refuse to remove '.' and '..') #6623

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

just-an-engineer
Copy link
Contributor

@just-an-engineer just-an-engineer commented Aug 7, 2024

Before processing the directory, if it ends with "." or "..", it now immediately returns with the same error message as GNU
Should close #6620

@just-an-engineer
Copy link
Contributor Author

I see this does the same thing as #6621

@sylvestre
Copy link
Contributor

sylvestre commented Aug 7, 2024

but tests/rm/r-4 is fixed in the other PR
(your modification is smaller than the other which is a +)

@just-an-engineer
Copy link
Contributor Author

Thanks. Do you want me to close this out? Or which PR will get merged?

@sylvestre
Copy link
Contributor

the other PR code is a bit more complex.

just-an-engineer and others added 2 commits August 8, 2024 11:25
Copy link

github-actions bot commented Aug 8, 2024

GNU testsuite comparison:

Congrats! The gnu test tests/timeout/timeout is no longer failing!

@sylvestre
Copy link
Contributor

@just-an-engineer what is your plan wrt this PR?

@just-an-engineer
Copy link
Contributor Author

We can merge this one if @AnirbanHalder654322 doesn't have any progress on the other PR, since I already have it working without regex, it already works on Windows, and since I made them a coauthor on the commits. But if they responds and pushes whatever changes, I'll leave it to you or them, but if the other one gets merged, I can just close this one.
So I can go either way. It can be merged right now, or it can be closed if Anirban pushes the necessary changes and you decide to go with that one

@just-an-engineer just-an-engineer marked this pull request as draft August 14, 2024 13:39
@just-an-engineer
Copy link
Contributor Author

@sylvestre, I'm marking this as a draft, and I'll close it out once @AnirbanHalder654322 PR gets merged. His is starting to look a bit cleaner and more fleshed out than mine. Particularly with removing trailing slashes, and more comprehensive tests, in my opinion.

src/uu/rm/src/rm.rs Outdated Show resolved Hide resolved
Comment on lines +331 to +337
if path
.to_str()
.unwrap()
.ends_with(format!("{}.", del).as_str())
|| path
.to_str()
.unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. The conversion is done twice. That's not a big deal, but maybe just saving the result in a variable is easy enough to avoid doing the same work twice?
  2. .to_str().unwrap() will cause problems with non-UTF-8 filenames. Please consider using os_str_as_bytes: If it works great (and add a test), if it doesn't (or is too much work or feels out of scope) then disregard this suggestion.
  3. I'm not sure whether this is the right condition. Maybe a trailing path separator ("del") should count, too? Here's a small example that demonstrates the difference:
    $ rm -rf foo && mkdir -p foo/bar/baz && rm -rf foo/bar/baz/../
    rm: refusing to remove '.' or '..' directory: skipping 'foo/bar/baz/../'
    [$? = 1]
    $ find foo/
    foo/
    foo/bar
    foo/bar/baz
    $ rm -rf foo && mkdir -p foo/bar/baz && cargo run -q --features rm rm -rf foo/bar/baz/../
    rm: cannot remove 'foo/bar/baz/../': No such file or directory (os error 2)
    [$? = 1]
    $ find foo
    foo
    foo/bar
    
    As you can see, we still delete a directory, causing a confusing error message. I'm not sure whether it suffices to add the two conditions ends_with("/./") and ".ends_with("/../")". Can you find any other edge cases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rm : Removes parent directory
3 participants