Skip to content

Commit

Permalink
Fixed unsoundness in StderrForwarder::forward_available (#1203)
Browse files Browse the repository at this point in the history
  • Loading branch information
NobodyXu authored Sep 7, 2024
1 parent 8df1156 commit 5e161c6
Showing 1 changed file with 16 additions and 13 deletions.
29 changes: 16 additions & 13 deletions src/command_helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ pub(crate) struct StderrForwarder {
is_non_blocking: bool,
#[cfg(feature = "parallel")]
bytes_available_failed: bool,
/// number of bytes buffered in inner
bytes_buffered: usize,
}

const MIN_BUFFER_CAPACITY: usize = 100;
Expand All @@ -105,6 +107,7 @@ impl StderrForwarder {
.stderr
.take()
.map(|stderr| (stderr, Vec::with_capacity(MIN_BUFFER_CAPACITY))),
bytes_buffered: 0,
#[cfg(feature = "parallel")]
is_non_blocking: false,
#[cfg(feature = "parallel")]
Expand All @@ -115,8 +118,6 @@ impl StderrForwarder {
fn forward_available(&mut self) -> bool {
if let Some((stderr, buffer)) = self.inner.as_mut() {
loop {
let old_data_end = buffer.len();

// For non-blocking we check to see if there is data available, so we should try to
// read at least that much. For blocking, always read at least the minimum amount.
#[cfg(not(feature = "parallel"))]
Expand Down Expand Up @@ -158,12 +159,11 @@ impl StderrForwarder {
} else {
MIN_BUFFER_CAPACITY
};
buffer.reserve(to_reserve);
if self.bytes_buffered + to_reserve > buffer.len() {
buffer.resize(self.bytes_buffered + to_reserve, 0);
}

// Safety: stderr.read only writes to the spare part of the buffer, it never reads from it
match stderr
.read(unsafe { &mut *(buffer.spare_capacity_mut() as *mut _ as *mut [u8]) })
{
match stderr.read(&mut buffer[self.bytes_buffered..]) {
Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => {
// No data currently, yield back.
break false;
Expand All @@ -173,22 +173,25 @@ impl StderrForwarder {
continue;
}
Ok(bytes_read) if bytes_read != 0 => {
// Safety: bytes_read bytes is written to spare part of the buffer
unsafe { buffer.set_len(old_data_end + bytes_read) };
self.bytes_buffered += bytes_read;
let mut consumed = 0;
for line in buffer.split_inclusive(|&b| b == b'\n') {
for line in buffer[..self.bytes_buffered].split_inclusive(|&b| b == b'\n') {
// Only forward complete lines, leave the rest in the buffer.
if let Some((b'\n', line)) = line.split_last() {
consumed += line.len() + 1;
write_warning(line);
}
}
buffer.drain(..consumed);
if consumed > 0 && consumed < self.bytes_buffered {
// Remove the consumed bytes from buffer
buffer.copy_within(consumed.., 0);
}
self.bytes_buffered -= consumed;
}
res => {
// End of stream: flush remaining data and bail.
if old_data_end > 0 {
write_warning(&buffer[..old_data_end]);
if self.bytes_buffered > 0 {
write_warning(&buffer[..self.bytes_buffered]);
}
if let Err(err) = res {
write_warning(
Expand Down

0 comments on commit 5e161c6

Please sign in to comment.