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

fix: insufficient buf size when reading windows named pipe message #1778

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dswij
Copy link

@dswij dswij commented Apr 27, 2024

Closes tokio-rs/tokio#6460

This PR allows handling ERROR_MORE_DATA when the internal buffer size is less than the message in NamedPipe. When ERROR_MORE_DATA is hit, the internal buffer will be resized according to PeekNamedPipe and the remaining message will be read through subsequent call to ReadFile.

Also see #1772 for more context.

@dswij dswij requested a review from carllerche as a code owner April 27, 2024 16:40
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add a test?

src/sys/windows/named_pipe.rs Show resolved Hide resolved
@dswij
Copy link
Author

dswij commented Apr 30, 2024

Ah, yeah. Forgot to set up formatting in the windows machine :)

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. Only things I'm wondering are:

  1. 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.
  2. 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.

@dswij
Copy link
Author

dswij commented May 9, 2024

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the original error recoverable?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original ERROR_MORE_DATA error is not really an error, it we actually read bytes, so it can be ignored.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, i thought you were referring to the err flow before the change.

Guess we can truncate it in this case

let mut buf = match mem::replace(&mut io.read, State::None) {
State::None => me.get_buffer(),
State::InsufficientBufferSize(mut buf, rem) => {
buf.reserve_exact(rem);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have a limit. Otherwise we can allocate large amounts of memory.

0,
std::ptr::null_mut(),
std::ptr::null_mut(),
&mut remaining,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case PeekNamedPipe returns 0 we should probably just return the short-read buffer, not an error.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic assert failed left: 4096 right: 0 - when sending > 4096 bytes in windows pipe message mode
3 participants