-
Notifications
You must be signed in to change notification settings - Fork 738
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
d4111ba
6d81622
ce98957
daa2379
43e3bae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,14 +7,14 @@ use std::sync::{Arc, Mutex}; | |
use std::{fmt, mem, slice}; | ||
|
||
use windows_sys::Win32::Foundation::{ | ||
ERROR_BROKEN_PIPE, ERROR_IO_INCOMPLETE, ERROR_IO_PENDING, ERROR_NO_DATA, ERROR_PIPE_CONNECTED, | ||
ERROR_PIPE_LISTENING, HANDLE, INVALID_HANDLE_VALUE, | ||
ERROR_BROKEN_PIPE, ERROR_IO_INCOMPLETE, ERROR_IO_PENDING, ERROR_MORE_DATA, ERROR_NO_DATA, | ||
ERROR_PIPE_CONNECTED, ERROR_PIPE_LISTENING, HANDLE, INVALID_HANDLE_VALUE, | ||
}; | ||
use windows_sys::Win32::Storage::FileSystem::{ | ||
ReadFile, WriteFile, FILE_FLAG_FIRST_PIPE_INSTANCE, FILE_FLAG_OVERLAPPED, PIPE_ACCESS_DUPLEX, | ||
}; | ||
use windows_sys::Win32::System::Pipes::{ | ||
ConnectNamedPipe, CreateNamedPipeW, DisconnectNamedPipe, PIPE_TYPE_BYTE, | ||
ConnectNamedPipe, CreateNamedPipeW, DisconnectNamedPipe, PeekNamedPipe, PIPE_TYPE_BYTE, | ||
PIPE_UNLIMITED_INSTANCES, | ||
}; | ||
use windows_sys::Win32::System::IO::{ | ||
|
@@ -307,6 +307,25 @@ impl Inner { | |
Ok(transferred as usize) | ||
} | ||
} | ||
|
||
/// Calls the `PeekNamedPipe` function to get the remaining size of message in NamedPipe | ||
#[inline] | ||
unsafe fn remaining_size(&self) -> io::Result<usize> { | ||
let mut remaining = 0; | ||
let r = PeekNamedPipe( | ||
self.handle.raw(), | ||
std::ptr::null_mut(), | ||
0, | ||
std::ptr::null_mut(), | ||
std::ptr::null_mut(), | ||
&mut remaining, | ||
); | ||
if r == 0 { | ||
Err(io::Error::last_os_error()) | ||
} else { | ||
Ok(remaining as usize) | ||
} | ||
} | ||
} | ||
|
||
#[test] | ||
|
@@ -349,6 +368,7 @@ enum State { | |
Pending(Vec<u8>, usize), | ||
Ok(Vec<u8>, usize), | ||
Err(io::Error), | ||
InsufficientBufferSize(Vec<u8>, usize), | ||
} | ||
|
||
// Odd tokens are for named pipes | ||
|
@@ -535,7 +555,7 @@ impl<'a> Read for &'a NamedPipe { | |
} | ||
|
||
// We previously read something into `data`, try to copy out some | ||
// data. If we copy out all the data schedule a new read and | ||
// data. If we copy out all the data, schedule a new read | ||
// otherwise store the buffer to get read later. | ||
State::Ok(data, cur) => { | ||
let n = { | ||
|
@@ -552,6 +572,10 @@ impl<'a> Read for &'a NamedPipe { | |
Ok(n) | ||
} | ||
|
||
// We scheduled another read with a bigger buffer after the first read (see `read_done`) | ||
// This is not possible in theory, just like `State::None` case, but return would block for now. | ||
State::InsufficientBufferSize(..) => Err(would_block()), | ||
|
||
// Looks like an in-flight read hit an error, return that here while | ||
// we schedule a new one. | ||
State::Err(e) => { | ||
|
@@ -703,19 +727,25 @@ impl Inner { | |
/// scheduled. | ||
fn schedule_read(me: &Arc<Inner>, io: &mut Io, events: Option<&mut Vec<Event>>) -> bool { | ||
// Check to see if a read is already scheduled/completed | ||
match io.read { | ||
State::None => {} | ||
_ => return true, | ||
} | ||
|
||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
buf | ||
} | ||
e @ _ => { | ||
io.read = e; | ||
return true; | ||
} | ||
}; | ||
|
||
// Allocate a buffer and schedule the read. | ||
let mut buf = me.get_buffer(); | ||
let e = unsafe { | ||
let overlapped = me.read.as_ptr() as *mut _; | ||
let slice = slice::from_raw_parts_mut(buf.as_mut_ptr(), buf.capacity()); | ||
me.read_overlapped(slice, overlapped) | ||
me.read_overlapped(&mut slice[buf.len()..], overlapped) | ||
}; | ||
|
||
match e { | ||
// See `NamedPipe::connect` above for the rationale behind `forget` | ||
Ok(_) => { | ||
|
@@ -874,9 +904,25 @@ fn read_done(status: &OVERLAPPED_ENTRY, events: Option<&mut Vec<Event>>) { | |
match me.result(status.overlapped()) { | ||
Ok(n) => { | ||
debug_assert_eq!(status.bytes_transferred() as usize, n); | ||
buf.set_len(status.bytes_transferred() as usize); | ||
// Extend the len depending on the initial len is necessary | ||
// when we call `ReadFile` again after resizing | ||
// our internal buffer | ||
buf.set_len(buf.len() + status.bytes_transferred() as usize); | ||
Darksonn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
io.read = State::Ok(buf, 0); | ||
} | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be moved before the call to |
||
io.read = State::InsufficientBufferSize(buf, rem); | ||
Inner::schedule_read(&me, &mut io, None); | ||
return; | ||
} | ||
Err(e) => { | ||
io.read = State::Err(e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is the original error recoverable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
} | ||
} | ||
Err(e) => { | ||
debug_assert_eq!(status.bytes_transferred(), 0); | ||
io.read = State::Err(e); | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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