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

walk: treat symbolic links to directories as directories #1397

Closed
wants to merge 1 commit into from

Conversation

nikofil
Copy link

@nikofil nikofil commented Oct 2, 2019

Due to how walkdir works if symlinks are not followed, symlinks to
directories are seen as simple files by ripgrep. This caused a panic
in some cases due to receiving a WalkEvent::Exit event without a
corresponding WalkEvent::Dir event.

This is fixed by looking at the metadata of the file in the case of a
symlink to determine if it's a directory.

Fixes #1389

Due to how walkdir works if symlinks are not followed, symlinks to
directories are seen as simple files by ripgrep. This caused a panic
in some cases due to receiving a WalkEvent::Exit event without a
corresponding WalkEvent::Dir event.

This is fixed by looking at the metadata of the file in the case of a
symlink to determine if it's a directory.

Fixes BurntSushi#1389

Signed-off-by: Nikos Filippakis <nikolaos.filippakis@cern.ch>
@BurntSushi
Copy link
Owner

Thanks. It's going to take me a bit to review this, as this adds an additional syscall for every symlink that is seen, as far as I can tell. We've had performance problems in the past (especially on Windows) when doing that.

@nikofil
Copy link
Author

nikofil commented Oct 2, 2019

I see, that could indeed be too much overhead.
The only case that this PR fixes should be the case reported (when the given path is a symlink to a directory). Otherwise when the option to follow symlinks isn't given the depth never changes so there are no WalkEvent::Exit events caused by symlinks:

ripgrep/ignore/src/walk.rs

Lines 1026 to 1030 in 8892bf6

if depth < self.depth {
self.depth -= 1;
self.next = dent;
return Some(Ok(WalkEvent::Exit));
}

Therefore checking only the given path, instead of any symlink encountered, should also fix this case. I can try fixing it that way if you agree.

Copy link
Owner

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Great, thanks! I think this looks good. I'll be merging it with some tweaks as part of #1486. In particular, I made it so the additional stat check only occurred when the directory entry had depth 0.

@BurntSushi BurntSushi added the rollup A PR that has been merged with many others in a rollup. label Feb 17, 2020
BurntSushi added a commit that referenced this pull request Feb 17, 2020
Due to how walkdir works if symlinks are not followed, symlinks to
directories are seen as simple files by ripgrep. This caused a panic
in some cases due to receiving a WalkEvent::Exit event without a
corresponding WalkEvent::Dir event.

This is fixed by looking at the metadata of the file in the case of a
symlink to determine if it's a directory. We are careful to only do
this stat check when the depth of the entry is 0, as this bug only
impacts us when 1) we aren't following symlinks generally and 2) the
user provides a symlinked directory that we do follow as a top-level
path to search.

Fixes #1389, Closes #1397
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rollup A PR that has been merged with many others in a rollup.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combination of --no-ignore, --sort, and search path of a symbolic link to a directory causes panic
2 participants