wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next #133184
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @Noratrieb rustbot has assigned @Noratrieb. Use |
rustbot
added
O-wasi
This comment has been minimized.
This comment has been minimized.
When upgrading [Zed](zed-industries/zed#19349) to Rust 1.82 I've encountered a test failure in our test suite. Specifically, one of our extension tests started hanging. I've tracked it down to a call to std::fs::remove_dir_all not returning when an extension is compiled with Rust 1.82 Our extension system uses WASM components, thus I've looked at the diff between 1.81 and 1.82 with respect to WASI and found 736f773 As it turned out, calling remove_dir_all from extension returned io::ErrorKind::NotFound in 1.81; the underlying issue is that the ReadDir iterator never actually terminates iteration, however since it loops around, with 1.81 we'd come across an entry second time and fail to remove it, since it would've been removed previously. With 1.82 and 736f773 it is no longer the case, thus we're seeing the hang. This commit makes ReadDir::next adhere to readdir contract, namely it will no longer call readdir once the returned # of bytes is smaller than the size of a passed-in buffer. Previously we'd only terminate the loop if readdir returned 0.
osiewicz
force-pushed
the
wasm-fix-infinite-loop-in-remove-dir-all
branch
from
954f70d to
529aae6
Compare
Noratrieb
reviewed
Nov 24, 2024
library/std/src/sys/pal/wasi/fs.rs
Outdated
| } else { | ||
| self.cookie = Some(cookie); | ||
| } | ||
| self.cap = self.buf.len(); |
There was a problem hiding this comment.
I don't think this is correct. In case readdir doesn't fill the entire buffer because there aren't many entries, this will still set the cap to the full buffer, causing us to overflow into the unfilled part and read garbage, since the data below will include it.
Or am I misunderstanding this function? It is very confusing, so I wouldn't be surprised.
There was a problem hiding this comment.
You're right in that self.cap should be bytes. I think though we would never read unfilled part, as readdir will read up to self.buf.len(), and unfilled Vec capacity is always filled out with 0s.
But yeah, this function as a whole if kinda confusing. I've tried to change as little of it as possible.
There was a problem hiding this comment.
Huh, changing that condition reintroduces the original hang.. O_o
There was a problem hiding this comment.
I feel like the best way to fix this is to rewrite it from scratch, with some more types and enums (explicit state machine maybe?)
Noratrieb
added
S-waiting-on-author
osiewicz
commented
Nov 26, 2024
rustbot
added
S-waiting-on-review
Noratrieb
approved these changes
Dec 7, 2024
|
@bors r+ |
bors
added
S-waiting-on-bors
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Dec 8, 2024
…-remove-dir-all, r=Noratrieb wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next When upgrading [Zed](zed-industries/zed#19349) to Rust 1.82 I've encountered a test failure in our test suite. Specifically, one of our extension tests started hanging. I've tracked it down to a call to std::fs::remove_dir_all not returning when an extension is compiled with Rust 1.82 Our extension system uses WASM components, thus I've looked at the diff between 1.81 and 1.82 with respect to WASI and found 736f773 As it turned out, calling remove_dir_all from extension returned io::ErrorKind::NotFound in 1.81; the underlying issue is that the ReadDir iterator never actually terminates iteration, however since it loops around, with 1.81 we'd come across an entry second time and fail to remove it, since it would've been removed previously. With 1.82 and 736f773 it is no longer the case, thus we're seeing the hang. The tests do pass when everything but the extensions is compiled with 1.82. This commit makes ReadDir::next adhere to readdir contract, namely it will no longer call readdir once the returned # of bytes is smaller than the size of a passed-in buffer. Previously we'd only terminate the loop if readdir returned 0.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 8, 2024
…kingjubilee Rollup of 5 pull requests Successful merges: - rust-lang#131669 (lint: change help for pointers to dyn types in FFI) - rust-lang#133184 (wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next) - rust-lang#133265 (Add a range argument to vec.extract_if) - rust-lang#133456 (Add licenses + Run `cargo update`) - rust-lang#133767 (Add more info on type/trait mismatches for different crate versions) Failed merges: - rust-lang#133522 (Don't suggest restricting bound with unstable traits on stable and mention it's unstable on nightly) r? `@ghost` `@rustbot` modify labels: rollup
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Dec 9, 2024
…-remove-dir-all, r=Noratrieb wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next When upgrading [Zed](zed-industries/zed#19349) to Rust 1.82 I've encountered a test failure in our test suite. Specifically, one of our extension tests started hanging. I've tracked it down to a call to std::fs::remove_dir_all not returning when an extension is compiled with Rust 1.82 Our extension system uses WASM components, thus I've looked at the diff between 1.81 and 1.82 with respect to WASI and found 736f773 As it turned out, calling remove_dir_all from extension returned io::ErrorKind::NotFound in 1.81; the underlying issue is that the ReadDir iterator never actually terminates iteration, however since it loops around, with 1.81 we'd come across an entry second time and fail to remove it, since it would've been removed previously. With 1.82 and 736f773 it is no longer the case, thus we're seeing the hang. The tests do pass when everything but the extensions is compiled with 1.82. This commit makes ReadDir::next adhere to readdir contract, namely it will no longer call readdir once the returned # of bytes is smaller than the size of a passed-in buffer. Previously we'd only terminate the loop if readdir returned 0.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 9, 2024
…llaumeGomez Rollup of 9 pull requests Successful merges: - rust-lang#133184 (wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next) - rust-lang#133955 (Pass the arch rather than full target name to windows_registry::find_tool) - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates) - rust-lang#133970 ([AIX] Replace sa_sigaction with sa_union.__su_sigaction for AIX) - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command) - rust-lang#133996 (Move most tests for `-l` and `#[link(..)]` into `tests/ui/link-native-libs`) - rust-lang#134017 (Don't use `AsyncFnOnce::CallOnceFuture` bounds for signature deduction) - rust-lang#134023 (handle cygwin environment in `install::sanitize_sh`) - rust-lang#134053 (rustdoc: rename `issue-\d+.rs` tests to have meaningful names (part 10)) r? `@ghost` `@rustbot` modify labels: rollup
jieyouxu
added a commit
to jieyouxu/rust
that referenced
this pull request
Dec 9, 2024
…-remove-dir-all, r=Noratrieb wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next When upgrading [Zed](zed-industries/zed#19349) to Rust 1.82 I've encountered a test failure in our test suite. Specifically, one of our extension tests started hanging. I've tracked it down to a call to std::fs::remove_dir_all not returning when an extension is compiled with Rust 1.82 Our extension system uses WASM components, thus I've looked at the diff between 1.81 and 1.82 with respect to WASI and found 736f773 As it turned out, calling remove_dir_all from extension returned io::ErrorKind::NotFound in 1.81; the underlying issue is that the ReadDir iterator never actually terminates iteration, however since it loops around, with 1.81 we'd come across an entry second time and fail to remove it, since it would've been removed previously. With 1.82 and 736f773 it is no longer the case, thus we're seeing the hang. The tests do pass when everything but the extensions is compiled with 1.82. This commit makes ReadDir::next adhere to readdir contract, namely it will no longer call readdir once the returned # of bytes is smaller than the size of a passed-in buffer. Previously we'd only terminate the loop if readdir returned 0.
jieyouxu
added a commit
to jieyouxu/rust
that referenced
this pull request
Dec 9, 2024
…-remove-dir-all, r=Noratrieb wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next When upgrading [Zed](zed-industries/zed#19349) to Rust 1.82 I've encountered a test failure in our test suite. Specifically, one of our extension tests started hanging. I've tracked it down to a call to std::fs::remove_dir_all not returning when an extension is compiled with Rust 1.82 Our extension system uses WASM components, thus I've looked at the diff between 1.81 and 1.82 with respect to WASI and found 736f773 As it turned out, calling remove_dir_all from extension returned io::ErrorKind::NotFound in 1.81; the underlying issue is that the ReadDir iterator never actually terminates iteration, however since it loops around, with 1.81 we'd come across an entry second time and fail to remove it, since it would've been removed previously. With 1.82 and 736f773 it is no longer the case, thus we're seeing the hang. The tests do pass when everything but the extensions is compiled with 1.82. This commit makes ReadDir::next adhere to readdir contract, namely it will no longer call readdir once the returned # of bytes is smaller than the size of a passed-in buffer. Previously we'd only terminate the loop if readdir returned 0.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 9, 2024
Rollup of 8 pull requests Successful merges: - rust-lang#133184 (wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next) - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates) - rust-lang#133970 ([AIX] Replace sa_sigaction with sa_union.__su_sigaction for AIX) - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command) - rust-lang#133996 (Move most tests for `-l` and `#[link(..)]` into `tests/ui/link-native-libs`) - rust-lang#134017 (Don't use `AsyncFnOnce::CallOnceFuture` bounds for signature deduction) - rust-lang#134023 (handle cygwin environment in `install::sanitize_sh`) - rust-lang#134053 (rustdoc: rename `issue-\d+.rs` tests to have meaningful names (part 10)) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 9, 2024
Rollup of 8 pull requests Successful merges: - rust-lang#133184 (wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next) - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates) - rust-lang#133970 ([AIX] Replace sa_sigaction with sa_union.__su_sigaction for AIX) - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command) - rust-lang#133996 (Move most tests for `-l` and `#[link(..)]` into `tests/ui/link-native-libs`) - rust-lang#134017 (Don't use `AsyncFnOnce::CallOnceFuture` bounds for signature deduction) - rust-lang#134023 (handle cygwin environment in `install::sanitize_sh`) - rust-lang#134053 (rustdoc: rename `issue-\d+.rs` tests to have meaningful names (part 10)) r? `@ghost` `@rustbot` modify labels: rollup
fmease
added a commit
to fmease/rust
that referenced
this pull request
Dec 9, 2024
…-remove-dir-all, r=Noratrieb wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next When upgrading [Zed](zed-industries/zed#19349) to Rust 1.82 I've encountered a test failure in our test suite. Specifically, one of our extension tests started hanging. I've tracked it down to a call to std::fs::remove_dir_all not returning when an extension is compiled with Rust 1.82 Our extension system uses WASM components, thus I've looked at the diff between 1.81 and 1.82 with respect to WASI and found 736f773 As it turned out, calling remove_dir_all from extension returned io::ErrorKind::NotFound in 1.81; the underlying issue is that the ReadDir iterator never actually terminates iteration, however since it loops around, with 1.81 we'd come across an entry second time and fail to remove it, since it would've been removed previously. With 1.82 and 736f773 it is no longer the case, thus we're seeing the hang. The tests do pass when everything but the extensions is compiled with 1.82. This commit makes ReadDir::next adhere to readdir contract, namely it will no longer call readdir once the returned # of bytes is smaller than the size of a passed-in buffer. Previously we'd only terminate the loop if readdir returned 0.
fmease
added a commit
to fmease/rust
that referenced
this pull request
Dec 9, 2024
…-remove-dir-all, r=Noratrieb wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next When upgrading [Zed](zed-industries/zed#19349) to Rust 1.82 I've encountered a test failure in our test suite. Specifically, one of our extension tests started hanging. I've tracked it down to a call to std::fs::remove_dir_all not returning when an extension is compiled with Rust 1.82 Our extension system uses WASM components, thus I've looked at the diff between 1.81 and 1.82 with respect to WASI and found 736f773 As it turned out, calling remove_dir_all from extension returned io::ErrorKind::NotFound in 1.81; the underlying issue is that the ReadDir iterator never actually terminates iteration, however since it loops around, with 1.81 we'd come across an entry second time and fail to remove it, since it would've been removed previously. With 1.82 and 736f773 it is no longer the case, thus we're seeing the hang. The tests do pass when everything but the extensions is compiled with 1.82. This commit makes ReadDir::next adhere to readdir contract, namely it will no longer call readdir once the returned # of bytes is smaller than the size of a passed-in buffer. Previously we'd only terminate the loop if readdir returned 0.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 10, 2024
Rollup of 10 pull requests Successful merges: - rust-lang#131558 (Lint on combining `#[no_mangle]` and `#[export_name]`) - rust-lang#133122 (Add unpolished, experimental support for AFIDT (async fn in dyn trait)) - rust-lang#133184 (wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next) - rust-lang#133456 (Add licenses + Run `cargo update`) - rust-lang#133472 (Run TLS destructors for wasm32-wasip1-threads) - rust-lang#133853 (use vendor sources by default on dist tarballs) - rust-lang#133946 (coverage: Prefer to visit nodes whose predecessors have been visited) - rust-lang#134010 (fix ICE on type error in promoted) - rust-lang#134029 (coverage: Use a query to find counters/expressions that must be zero) - rust-lang#134071 (Configure renovatebot) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 10, 2024
Rollup of 10 pull requests Successful merges: - rust-lang#131558 (Lint on combining `#[no_mangle]` and `#[export_name]`) - rust-lang#133184 (wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next) - rust-lang#133456 (Add licenses + Run `cargo update`) - rust-lang#133472 (Run TLS destructors for wasm32-wasip1-threads) - rust-lang#133853 (use vendor sources by default on dist tarballs) - rust-lang#133946 (coverage: Prefer to visit nodes whose predecessors have been visited) - rust-lang#134010 (fix ICE on type error in promoted) - rust-lang#134029 (coverage: Use a query to find counters/expressions that must be zero) - rust-lang#134071 (Configure renovatebot) - rust-lang#134102 (Miscellaneous fixes for nix-dev-shell) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 10, 2024
Rollup merge of rust-lang#133184 - osiewicz:wasm-fix-infinite-loop-in-remove-dir-all, r=Noratrieb wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next When upgrading [Zed](zed-industries/zed#19349) to Rust 1.82 I've encountered a test failure in our test suite. Specifically, one of our extension tests started hanging. I've tracked it down to a call to std::fs::remove_dir_all not returning when an extension is compiled with Rust 1.82 Our extension system uses WASM components, thus I've looked at the diff between 1.81 and 1.82 with respect to WASI and found 736f773 As it turned out, calling remove_dir_all from extension returned io::ErrorKind::NotFound in 1.81; the underlying issue is that the ReadDir iterator never actually terminates iteration, however since it loops around, with 1.81 we'd come across an entry second time and fail to remove it, since it would've been removed previously. With 1.82 and 736f773 it is no longer the case, thus we're seeing the hang. The tests do pass when everything but the extensions is compiled with 1.82. This commit makes ReadDir::next adhere to readdir contract, namely it will no longer call readdir once the returned # of bytes is smaller than the size of a passed-in buffer. Previously we'd only terminate the loop if readdir returned 0.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When upgrading Zed to Rust 1.82 I've encountered a test failure in our test suite. Specifically, one of our extension tests started hanging. I've tracked it down to a call to std::fs::remove_dir_all not returning when an extension is compiled with Rust 1.82 Our extension system uses WASM components, thus I've looked at the diff between 1.81 and 1.82 with respect to WASI and found 736f773
As it turned out, calling remove_dir_all from extension returned io::ErrorKind::NotFound in 1.81; the underlying issue is that the ReadDir iterator never actually terminates iteration, however since it loops around, with 1.81 we'd come across an entry second time and fail to remove it, since it would've been removed previously. With 1.82 and 736f773 it is no longer the case, thus we're seeing the hang. The tests do pass when everything but the extensions is compiled with 1.82.
This commit makes ReadDir::next adhere to readdir contract, namely it will no longer call readdir once the returned # of bytes is smaller than the size of a passed-in buffer. Previously we'd only terminate the loop if readdir returned 0.