elliottt requested alexcrichton for a review on PR #8611.
elliottt requested wasmtime-core-reviewers for a review on PR #8611.
elliottt opened PR #8611 from elliottt:trevor/wasi-file-stream-read-zero
to bytecodealliance:main
:
The
read
method on streams should support a size argument of zero, to test if the stream is still open. However, the implementation ofread
for open files was unconditionally treating a read that would return zero bytes as an indication that the stream was closed.This PR fixes this bug by handling a
read(0)
as an explicit test to see if the current position is at the end of the file.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
elliottt updated PR #8611.
alexcrichton submitted PR review:
I'm a little hesitant to provide such a strong guarantee that a read of size 0 is effectively a
stat
on all files. Would it work to instead pass the read size toread_result
and don't returnClosed
if the read size is zero?
elliottt commented on PR #8611:
I don't think that works, as
cap_std::fs::File::read_at
won't signal any error for a read off the end of0
bytes. That means that aread(0)
will succeed indicating that it ready zero bytes, and a subsequent non-zero read will fail because the file is closed. I agree that this is a bit heavyweight, but I think something like it is necessary to satisfy this part of the streams contract: https://github.com/bytecodealliance/wasmtime/blob/7a91375623051da84e60f513564a144078eeb43c/crates/wasi/wit/deps/io/streams.wit#L51-L53
elliottt edited a comment on PR #8611:
I don't think that works, as
cap_std::fs::File::read_at
won't signal any error for a read at the end of a file, of0
bytes. That means that aread(0)
will succeed indicating that it read zero bytes, and a subsequent non-zero read will fail because the file is closed. I agree that this is a bit heavyweight, but I think something like it is necessary to satisfy this part of the streams contract: https://github.com/bytecodealliance/wasmtime/blob/7a91375623051da84e60f513564a144078eeb43c/crates/wasi/wit/deps/io/streams.wit#L51-L53
alexcrichton commented on PR #8611:
I think it still satisfies the contract though because the streams interface is supposed to be super general and it's always possible for a generic stream to get disconnected at any time. For example if you do a zero-length read on a socket it may say it's ready only for the next read to say it's closed.
elliottt updated PR #8611.
elliottt commented on PR #8611:
Fair enough. I've pushed a change to that effect.
alexcrichton submitted PR review.
elliottt updated PR #8611.
elliottt merged PR #8611.
Last updated: Dec 23 2024 at 13:07 UTC