fix: insufficient buf size when reading windows named pipe message#1778
fix: insufficient buf size when reading windows named pipe message#1778dswij wants to merge 6 commits intotokio-rs:masterfrom
Conversation
Darksonn
left a comment
Darksonn
left a comment
There was a problem hiding this comment.
Is it possible to add a test?
|
Ah, yeah. Forgot to set up formatting in the windows machine :) |
dswij
force-pushed
the
handle-err-more-data
branch
2 times, most recently
from
184703b to
ce98957
Compare
Darksonn
left a comment
Darksonn
left a comment
There was a problem hiding this comment.
Overall LGTM. Only things I'm wondering are:
- Can we add a test that sends several messages? E.g., generate some sequence of lengths to send, then spawn a background thread that sends messages of those lengths in those orders. Then on the main test thread, receive the messages and assert that the received lengths match.
- Can you run Tokio's test suite with these changes?
We also need @Thomasdezeeuw's ok, but with the above, I would be happy with this PR.
|
The change passed Tokio's test suites, but the CI is failing on unexpected cfgs. I opened a PR to handle this, unless you have other ideas for the unexpected cfgs @Thomasdezeeuw @Darksonn |
| Err(e) if e.raw_os_error() == Some(ERROR_MORE_DATA as i32) => { | ||
| match me.remaining_size() { | ||
| Ok(rem) => { | ||
| buf.set_len(status.bytes_transferred() as usize); |
There was a problem hiding this comment.
I think this should be moved before the call to remaining_size as those bytes are already read.
| return; | ||
| } | ||
| Err(e) => { | ||
| io.read = State::Err(e); |
There was a problem hiding this comment.
I'm not sure about this. This makes it an unrecoverable error, but the original error is not. I feel like this is making the situation worse in case PeekNamedPipe returns an error.
There was a problem hiding this comment.
How is the original error recoverable?
There was a problem hiding this comment.
The original ERROR_MORE_DATA error is not really an error, it we actually read bytes, so it can be ignored.
There was a problem hiding this comment.
Ah ok, i thought you were referring to the err flow before the change.
Guess we can truncate it in this case
There was a problem hiding this comment.
Changed here to truncate instead of returning Err: 08d9719
But now that I think about it, I'm not sure we'd want to ignore it without propagating the error to the user. It seems like a bad design.
| 0, | ||
| std::ptr::null_mut(), | ||
| std::ptr::null_mut(), | ||
| &mut remaining, |
There was a problem hiding this comment.
Is it guaranteed that ERROR_MORE_DATA is never returned for stream oriented named pipes?
Because according to the PeekNamedPipe docs (https://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-peeknamedpipe) this will return 0 for streams. Which would mean we're effectively creating an infinite loop where we try to read using a buffer of length 0.
There was a problem hiding this comment.
I'd be surprised if ERROR_MORE_DATA is ever returned by stream mode named pipe.
But that's a fair point. We can just err out if PeekNamedPipe returned 0.
There was a problem hiding this comment.
In case PeekNamedPipe returns 0 we should probably just return the short-read buffer, not an error.
There was a problem hiding this comment.
That will happen only when it's stream mode, and we do not expect it to happen, no?
I think we should at least have a debug_assert in this case
dswij
force-pushed
the
handle-err-more-data
branch
from
43e3bae to
6934a16
Compare
3 participants
Closes tokio-rs/tokio#6460
This PR allows handling
ERROR_MORE_DATAwhen the internal buffer size is less than the message in NamedPipe. WhenERROR_MORE_DATAis hit, the internal buffer will be resized according toPeekNamedPipeand the remaining message will be read through subsequent call toReadFile.Also see #1772 for more context.