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

Restrict the preview1 implementation of fd_read to one iovec #8415

Merged

Conversation

elliottt
Copy link
Member

@elliottt elliottt commented Apr 19, 2024

In the wasi-common implementation of fd_read for preview1, switch to only reading the first iovec given in a vectored read. This avoids issues with Wiggle's runtime borrow-checker implemented in #8277, which would cause the reads to unconditionally fail when two mutable borrows were taken out on the same memory.

While it's not as efficient to perform a single read at a time, it is also valid, as read never promises to read all of the vectors provided.

Co-authored-by: Nick Fitzgerald fitzgen@gmail.com

@elliottt elliottt marked this pull request as ready for review April 19, 2024 18:47
@elliottt elliottt requested a review from a team as a code owner April 19, 2024 18:47
@elliottt elliottt requested review from alexcrichton and a team and removed request for a team April 19, 2024 18:47
@alexcrichton
Copy link
Member

I was initially confused reading over this trying to piece together how this wasn't covered by #8353 but that was fd_pread rather than fd_read.

Mind adding a test for this? Also, can you update the filter here to be the same as #8353 where it searches for the first non-empty buffer rather than the first buffer?

@github-actions github-actions bot added the wasi Issues pertaining to WASI label Apr 19, 2024
…8277

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
@elliottt elliottt force-pushed the trevor/fix-p1-fd-read-mut-borrows branch from 60df49b to 903aff1 Compare April 19, 2024 21:20
@elliottt
Copy link
Member Author

I added a test, and verified that it failed without the fix to fd_read. (It's the same as the pread/pwrite test, but using fd_seek to polyfill their behavior.)

@elliottt elliottt force-pushed the trevor/fix-p1-fd-read-mut-borrows branch from 903aff1 to 27e1a96 Compare April 19, 2024 21:28
@elliottt elliottt force-pushed the trevor/fix-p1-fd-read-mut-borrows branch from 27e1a96 to d25e9b2 Compare April 19, 2024 21:29
@elliottt elliottt added this pull request to the merge queue Apr 19, 2024
Merged via the queue into bytecodealliance:main with commit 8edbfea Apr 19, 2024
21 checks passed
@elliottt elliottt deleted the trevor/fix-p1-fd-read-mut-borrows branch April 19, 2024 22:46
alexcrichton pushed a commit to alexcrichton/wasmtime that referenced this pull request Apr 22, 2024
…ealliance#8415)

* We can only have one mutable borrow at a time after bytecodealliance#8277

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

* Add a test like pread/pwrite, but for read/write

---------

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
fitzgen added a commit that referenced this pull request Apr 22, 2024
…8431)

* We can only have one mutable borrow at a time after #8277



* Add a test like pread/pwrite, but for read/write

---------

Co-authored-by: Trevor Elliott <telliott@fastly.com>
Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants