-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Make the parallel walker visit untraversable directories before failing #1365
Conversation
This commit fixes an inconsistency between the serial and the parallel directory walkers around visiting a directory for which the user holds insufficient permissions to descend into. The serial walker does produce a successful entry for a directory that it cannot descend into due to insufficient permissions. However, before this change that has not been the case for the parallel walker, which would produce an `Err` item not only when descending into a directory that it cannot read from but also for the directory entry itself. This change brings the behaviour of the parallel variant in line with that of the serial one.
329e481
to
a08a1b1
Compare
I should probably note that the new test will always fail when running it as a root. Is that a problem? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this!
let builder = WalkBuilder::new(td.path()); | ||
assert_paths(td.path(), &builder, &["a"]); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah, tests should not fail depending on what user ran them. Maybe it would be good to check if the file is readable after chmodding it. If it is, then skip the test.
With that said, it would be nice if we could avoid bringing in libc
and unsafe
for this test. Is there some other way to test this? Nothing is coming to mind... Maybe using a pre-existing file that a normal user traditionally doesn't have access to? (And skipping the test if the file doesn't exist or is readable.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also make sure that lines are wrapped to 79 columns, inclusive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BurntSushi, I added the proposed check, though, of course, it'd be better if the harness allowed marking a test as skipped at runtime.
With regard to avoiding adding a dev-dependency on libc
I don't have a good idea either. But now I made it a Linux-only dependency. Perhaps that's good enough?
If using libc
is really an issue (even though it already is a regular dependency for some other ripgrep crates, though, in truth, not ignore
itself), then I'll be happy to change this to try to walk /root and check if it is observing at least one valid entry (i.e. the one for the directory itself).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably care more about the extra unsafe
to be honest. I'm mostly just trying to make sure things are as a tight as possible. This isn't a huge deal, but I want to try for an alternative if it's feasible.
Just looking at my file system, maybe just trying look for /etc/sudoers.d
? It's readable by root
, but not by anyone else. So I think the logic might be something like this:
- Stat
/etc/sudoers.d
viastd::fs::metadata()
. - If it errors, skip test.
- Read
/etc/sudoers.d
. - If it succeeds, skip test.
- Try to traverse
/etc/sudoers.d
and run the test normally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BurntSushi, alright, sounds like a plan! I pushed the proposed change. Without a doubt that made the test much shorter and simpler. Thanks for the review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe /root
(or more generally ~root
) is more ubiquitous than /etc/sudoers.d
?
4694d52
to
d285c2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think this LGTM. I'll merge this once I get CI back online. (I tried setting up GitHub Actions over the weekend, but it's completely stuck.)
@BurntSushi, there's a button there for cancelling a workflow. Have you tried that? (You probably have and I'm asking a stupid question. 🙂) |
Hah, I wish. But yeah, that button does normally show up. In my case though, Actions seems to be stuck in some sort of initialization state where by that button doesn't show up. The jobs have been running for over 4 days too, which suggests timeout functionality doesn't work either. I've sent mail to GitHub support but haven't heard back from them. I've turned Travis and AppVeyor back on. Going to see if I can trigger it on this PR. |
there is a subtle bug and a performance issue. give me some minutes and I will explain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that there is a small bug and a little persormance issue. (one is inside a piece marked as resolved)
// have sufficient read permissions to list the directory. | ||
// In that case we still want to provide the closure with a valid | ||
// entry before passing the error value. | ||
let readdir = work.read_dir(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here there is an unnecessary syscall. I think that a clone is 99% better in performance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow your comment here. There is exactly one call to read_dir
both before and after this PR.
This commit fixes an inconsistency between the serial and the parallel directory walkers around visiting a directory for which the user holds insufficient permissions to descend into. The serial walker does produce a successful entry for a directory that it cannot descend into due to insufficient permissions. However, before this change that has not been the case for the parallel walker, which would produce an `Err` item not only when descending into a directory that it cannot read from but also for the directory entry itself. This change brings the behaviour of the parallel variant in line with that of the serial one. Fixes #1346, Closes #1365
This PR fixes an inconsistency between the serial and the parallel
directory walkers around visiting a directory for which the user holds
insufficient permissions to descend into.
The serial walker does produce a successful entry for a directory that
it cannot descend into due to insufficient permissions. However, before
this change that has not been the case for the parallel walker, which
would produce an
Err
item not only when descending into a directorythat it cannot read from but also for the directory entry itself.
This change brings the behaviour of the parallel variant in line with
that of the serial one.
Fixes #1346.