Fix possibly endless loop in ReadDir iterator #50630

sharkdp · 2018-05-10T20:24:13Z

Certain directories in /proc can cause the ReadDir iterator to loop indefinitely. We get an error code (22) when calling libc's readdir_r on these directories, but entry_ptr is NULL at the same time, signalling the end of the directory stream.

This change introduces an internal state to the iterator such that the Some(Err(..)) value will only be returned once when calling next. Subsequent calls will return None.

fixes #50619

rust-highfive · 2018-05-10T20:24:22Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

ishitatsuyuki · 2018-05-11T04:16:20Z

Can we add a test for this?

nagisa · 2018-05-11T15:03:26Z

src/libstd/sys/unix/fs.rs

@@ -51,6 +51,7 @@ pub struct FileAttr {
 pub struct ReadDir {
    dirp: Dir,
    root: Arc<PathBuf>,
+    end_of_stream: bool,


I think it might be better to just wrap Dir into an Option (and mark the pointer within Dir as NonZero) as that would both be more semantically accurate, more efficient space wise and more future-proof in terms of preventing mistakes.

nagisa · 2018-05-11T15:18:41Z

Adding a test for this would be fairly difficult, as the original reproducer relies on a fairly specific state of the system that would be pretty hard to emulate in way that wouldn’t spuriously fail all the time.

sharkdp · 2018-05-12T14:03:27Z

I have updated the code to use Option<Dir> and NonNull<libc::DIR> instead of the additional bool flag, as per @nagisa's excellent suggestion.

However, I'm not too happy about my code changes in the next function. It is slightly awkward to pattern-match on dirp: Option<Dir> while having to mutate dirp at the same time. If someone has a better suggestion on how to write this function, I'd be happy to change this again.

@ishitatsuyuki Concerning tests: I would love to write a test for this, but I'm really not sure how to. I also share the same view as @nagisa. Note that there is a way to reproduce the bug in the attached ticket (#50619).

sfackler · 2018-05-21T04:15:03Z

src/libstd/sys/unix/fs.rs

-                }
-                if ret.name_bytes() != b"." && ret.name_bytes() != b".." {
-                    return Some(Ok(ret))
+        if self.dirp.is_none() {


Don't we make this check again just below on 259?

Oh, absolutely - I will remove this if clause.

sfackler · 2018-05-21T04:15:19Z

src/libstd/sys/unix/fs.rs

@@ -49,11 +49,11 @@ pub struct FileAttr {
 }

 pub struct ReadDir {
-    dirp: Dir,
+    dirp: Option<Dir>,


Is this change related to the infinite loop issue?

Yes. Option is used to store whether we are at the end of the directory stream (Some(..)) or not (None).

sfackler · 2018-05-21T04:16:49Z

src/libstd/sys/unix/fs.rs

+                        if readdir64_r(dirp.as_mut().unwrap().0.as_mut(),
+                                       &mut ret.entry,
+                                       &mut entry_ptr) != 0 {
+                            if entry_ptr.is_null() {


This is the actual change being made, right?

Yes, exactly. If readdir64_r returns a non-empty exit code and signals the end of the directory stream by writing nullptr to entry_ptr, we still want to return the OS error in this iteration but None on the next iteration.

I think this change can be made a bit simpler by just using a null pointer instead of the whole option business. Then the diff just becomes a if self.dirp.0.is_null() { return None; } at the top of Next, and an if entry_ptr.is_null() { self.dirp.0 = ptr::null_mut(); } in the error case here.

pietroalbini · 2018-05-28T08:53:31Z

Ping from triage @sfackler! The author pushed some new commits.

bors · 2018-05-31T19:57:26Z

☔ The latest upstream changes (presumably #51050) made this pull request unmergeable. Please resolve the merge conflicts.

nagisa · 2018-06-01T09:18:24Z

Option ensures that other areas of code properly handle the null case as well. Both for this PR and the future changes.

…

On Fri, Jun 1, 2018, 08:02 Steven Fackler ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/libstd/sys/unix/fs.rs <#50630 (comment)>: > + } + + match self.dirp { + None => return None, + ref mut dirp @ Some(_) => { + unsafe { + let mut ret = DirEntry { + entry: mem::zeroed(), + root: self.root.clone() + }; + let mut entry_ptr = ptr::null_mut(); + loop { + if readdir64_r(dirp.as_mut().unwrap().0.as_mut(), + &mut ret.entry, + &mut entry_ptr) != 0 { + if entry_ptr.is_null() { I think this change can be made a bit simpler by just using a null pointer instead of the whole option business. Then the diff just becomes a if self.dirp.0.is_null() { return None; } at the top of Next, and an if entry_ptr.is_null() { self.dirp.0 = ptr::null_mut(); } in the error case here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50630 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApc0oad4EwvkWFqqyB_XeHankt2chd8ks5t4MrrgaJpZM4T6fvb> .

sharkdp · 2018-06-01T09:32:03Z

Option ensures that other areas of code properly handle the null case as well.

Exactly. Also, I'm personally fine with the solution suggested by @sfackler but I'm not sure if we should actively use nullptr to modify control flow ("actively" meaning that we set dirp to nullptr in cases where the C code does not).

Let me know whether I should go forward with @sfackler's suggestion or resolve the merge conflicts in the current version.

nagisa · 2018-06-01T15:28:04Z

Just rebase it, I’ll r+ it.

sharkdp · 2018-06-02T09:58:48Z

Just rebase it

I tried and failed. The whole ReadDir (or now: InnerReadDir) struct has been moved inside an Arc (in this PR: #51050), which means that I can not mutate parts of it(?).

To be honest, I don't really understand why it has been moved into the Arc (Note that the underlying DIR pointer will still be mutated via readdir64_r).

As it stands, I either need some help to make this work again (my attempt at resolving the conflicts can be found here: https://gist.github.com/sharkdp/fd60040daf25630f67f095d0fa616b82 - it currently does not pass the borrow-checker) or I move forward with @sfackler's suggestion, which should still work.

nagisa · 2018-06-02T11:31:13Z

Well, I assume that Dir is now within an Arc specifically so that it is freed only when both ReadDir and all DirEntryies are out of scope (as calling metadata on DirEntry requires a valid open Dir).

The problem is, of course, that now we have two distinct, and colliding requirements. First, we must somehow maintain information about when Dir has completed reading (preferably by setting the Dir field of ReadDir to None), but must as well keep Dir around until all of the DirEntryies are dropped (so that DirEntry is able to call the metadata method). One option would be a

struct DirEntry { 
    dir: Option<Arc<Dir>>,
    root: Arc<PathBuf>,
}

but that seems to increasingly stray away from what should be a 0-cost abstraction.

Quick and dirty alternative is to just add a boolean flag to the struct now, but I’d prefer not to land code like that if there’s any way to avoid it.

TimNN · 2018-06-12T17:27:49Z

Friendly ping from triage, @sharkdp, it looks like this PR needs some additional design / implementation work. Do you think you'll be able to look into that?

Certain directories in `/proc` can cause the `ReadDir` iterator to loop indefinitely. We get an error code (22) when calling libc's `readdir_r` on these directories, but `entry_ptr` is `NULL` at the same time, signalling the end of the directory stream. This change introduces an internal state to the iterator such that the `Some(Err(..))` value will only be returned once when calling `next`. Subsequent calls will return `None`. fixes #50619

sharkdp · 2018-06-12T19:11:14Z

@TimNN I am happy to work on this, but I will probably need some help, as mentioned above.

I have now pushed a new commit which resolves the merge conflicts and handles the problem in the same way as in my first commit - with an additional flag. This is a rather minimal change to fix this bug, but I agree with @nagisa that it is probably not optimal. However, I don't see how the other two discussed solutions (using Option or nullptr to store the state) can still work with the new changes on master (where the inner part of ReadDir is wrapped in an Arc).

pietroalbini · 2018-06-18T09:06:20Z

Ping from triage @sfackler! The author needs a bit of help, could you check in on this?

pietroalbini · 2018-06-25T08:29:38Z

Ping from triage @sfackler! The author needs a bit of help, could you check in on this?

sfackler · 2018-06-26T02:50:29Z

Oops, sorry. This seems good to me!

@bors r+

bors · 2018-06-26T02:50:30Z

📌 Commit af75314 has been approved by sfackler

bors · 2018-06-26T03:49:43Z

⌛ Testing commit af75314 with merge 7d2fa4a...

Fix possibly endless loop in ReadDir iterator Certain directories in `/proc` can cause the `ReadDir` iterator to loop indefinitely. We get an error code (22) when calling libc's `readdir_r` on these directories, but `entry_ptr` is `NULL` at the same time, signalling the end of the directory stream. This change introduces an internal state to the iterator such that the `Some(Err(..))` value will only be returned once when calling `next`. Subsequent calls will return `None`. fixes #50619

bors · 2018-06-26T05:56:09Z

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing 7d2fa4a to master...

This upgrades the minimum required version of Rust to 1.29 in order to fix BurntSushi#916 (Zombie processes cause `rg --files` to hang in `/proc`). See also: - Rust compiler bug ticket: rust-lang/rust#50619 - Rust compiler PR with the fix: rust-lang/rust#50630 closes BurntSushi#916

This upgrades the minimum required version of Rust to 1.29 in order to fix #288. See also: - Rust compiler bug ticket: rust-lang/rust#50619 - Rust compiler PR with the fix: rust-lang/rust#50630 closes #288

Bug rust-lang#50619 was fixed by adding an end_of_stream flag in rust-lang#50630. Unfortunately, that fix only applied to the readdir_r() path. When I switched Linux to use readdir() in rust-lang#92778, I inadvertently reintroduced the bug on that platform. Other platforms that had always used readdir() were presumably never fixed. This patch enables end_of_stream for all platforms, and adds a Linux-specific regression test that should hopefully prevent the bug from being reintroduced again.

…imulacrum fs: Fix rust-lang#50619 (again) and add a regression test Bug rust-lang#50619 was fixed by adding an end_of_stream flag in rust-lang#50630. Unfortunately, that fix only applied to the readdir_r() path. When I switched Linux to use readdir() in rust-lang#92778, I inadvertently reintroduced the bug on that platform. Other platforms that had always used readdir() were presumably never fixed. This patch enables end_of_stream for all platforms, and adds a Linux-specific regression test that should hopefully prevent the bug from being reintroduced again.

rust-highfive assigned sfackler May 10, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2018

nagisa reviewed May 11, 2018

View reviewed changes

sfackler reviewed May 21, 2018

View reviewed changes

TimNN added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 12, 2018

pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 25, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2018

bors merged commit af75314 into rust-lang:master Jun 26, 2018

sharkdp mentioned this pull request Jul 2, 2018

fd can get stuck when ran onto the whole FS from the root sharkdp/fd#288

Closed

sharkdp mentioned this pull request Sep 17, 2018

Upgrade minimum reqiured version of Rust to 1.29 BurntSushi/ripgrep#1058

Closed

sharkdp mentioned this pull request Sep 17, 2018

Upgrade minimum reqiured version of Rust to 1.29 sharkdp/fd#325

Merged

sharkdp mentioned this pull request Oct 27, 2018

fd hangs after accepting entry in fuzz finder sharkdp/fd#353

Closed

tavianator mentioned this pull request Dec 12, 2022

fs: Fix #50619 (again) and add a regression test #105638

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix possibly endless loop in ReadDir iterator #50630

Fix possibly endless loop in ReadDir iterator #50630

sharkdp commented May 10, 2018

rust-highfive commented May 10, 2018

ishitatsuyuki commented May 11, 2018

nagisa May 11, 2018

nagisa commented May 11, 2018

sharkdp commented May 12, 2018

sfackler May 21, 2018

sharkdp May 21, 2018

sfackler May 21, 2018

sharkdp May 21, 2018 •

edited

Loading

sfackler May 21, 2018

sharkdp May 21, 2018 •

edited

Loading

sfackler Jun 1, 2018

pietroalbini commented May 28, 2018

bors commented May 31, 2018

nagisa commented Jun 1, 2018 via email

sharkdp commented Jun 1, 2018

nagisa commented Jun 1, 2018

sharkdp commented Jun 2, 2018

nagisa commented Jun 2, 2018

TimNN commented Jun 12, 2018

sharkdp commented Jun 12, 2018

pietroalbini commented Jun 18, 2018

pietroalbini commented Jun 25, 2018

sfackler commented Jun 26, 2018

bors commented Jun 26, 2018

bors commented Jun 26, 2018

bors commented Jun 26, 2018

Fix possibly endless loop in ReadDir iterator #50630

Fix possibly endless loop in ReadDir iterator #50630

Conversation

sharkdp commented May 10, 2018

rust-highfive commented May 10, 2018

ishitatsuyuki commented May 11, 2018

nagisa May 11, 2018

Choose a reason for hiding this comment

nagisa commented May 11, 2018

sharkdp commented May 12, 2018

sfackler May 21, 2018

Choose a reason for hiding this comment

sharkdp May 21, 2018

Choose a reason for hiding this comment

sfackler May 21, 2018

Choose a reason for hiding this comment

sharkdp May 21, 2018 • edited Loading

Choose a reason for hiding this comment

sfackler May 21, 2018

Choose a reason for hiding this comment

sharkdp May 21, 2018 • edited Loading

Choose a reason for hiding this comment

sfackler Jun 1, 2018

Choose a reason for hiding this comment

pietroalbini commented May 28, 2018

bors commented May 31, 2018

nagisa commented Jun 1, 2018 via email

sharkdp commented Jun 1, 2018

nagisa commented Jun 1, 2018

sharkdp commented Jun 2, 2018

nagisa commented Jun 2, 2018

TimNN commented Jun 12, 2018

sharkdp commented Jun 12, 2018

pietroalbini commented Jun 18, 2018

pietroalbini commented Jun 25, 2018

sfackler commented Jun 26, 2018

bors commented Jun 26, 2018

bors commented Jun 26, 2018

bors commented Jun 26, 2018

sharkdp May 21, 2018 •

edited

Loading

sharkdp May 21, 2018 •

edited

Loading