From 8ff1f2e4e05aa60fde1063dc407b301ecac9f617 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 2 Jun 2024 16:19:55 +1000 Subject: [PATCH 1/5] Fix panic safety issue in StderrForwarder Fixed #1036 --- src/command_helpers.rs | 43 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/src/command_helpers.rs b/src/command_helpers.rs index fe919a523..266316a1a 100644 --- a/src/command_helpers.rs +++ b/src/command_helpers.rs @@ -104,7 +104,7 @@ impl StderrForwarder { let to_reserve = if self.is_non_blocking && !self.bytes_available_failed { match crate::parallel::stderr::bytes_available(stderr) { #[cfg(windows)] - Ok(0) => return false, + Ok(0) => break false, #[cfg(unix)] Ok(0) => { // On Unix, depending on the implementation, we may sometimes get 0 in a @@ -120,7 +120,7 @@ impl StderrForwarder { write_warning(&buffer[..]); } self.inner = None; - return true; + break true; } #[cfg(unix)] Err(_) => { @@ -137,32 +137,19 @@ impl StderrForwarder { }; buffer.reserve(to_reserve); - // SAFETY: 1) the length is set to the capacity, so we are never using memory beyond - // the underlying buffer and 2) we always call `truncate` below to set the len back - // to the initialized data. - unsafe { - buffer.set_len(buffer.capacity()); - } - match stderr.read(&mut buffer[old_data_end..]) { + // 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]) }) { Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => { // No data currently, yield back. - buffer.truncate(old_data_end); - return false; + break false; } Err(err) if err.kind() == std::io::ErrorKind::Interrupted => { // Interrupted, try again. - buffer.truncate(old_data_end); + continue; } - Ok(0) | Err(_) => { - // End of stream: flush remaining data and bail. - if old_data_end > 0 { - write_warning(&buffer[..old_data_end]); - } - self.inner = None; - return true; - } - Ok(bytes_read) => { - buffer.truncate(old_data_end + bytes_read); + 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) }; let mut consumed = 0; for line in buffer.split_inclusive(|&b| b == b'\n') { // Only forward complete lines, leave the rest in the buffer. @@ -173,6 +160,18 @@ impl StderrForwarder { } buffer.drain(..consumed); } + res => { + // End of stream: flush remaining data and bail. + if old_data_end > 0 { + write_warning(&buffer[..old_data_end]); + } + + let child_pid = self.inner.take().unwrap().id(); + if let Err(err) = res { + write_warning(format!("Failed to read from child {id} stderr: {err}").as_bytes()); + } + break true; + } } } } else { From daa00eab8daeb05dd4adae22592e549368b3d15a Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 2 Jun 2024 16:21:43 +1000 Subject: [PATCH 2/5] fix compilation --- src/command_helpers.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/command_helpers.rs b/src/command_helpers.rs index 266316a1a..28fdbbc69 100644 --- a/src/command_helpers.rs +++ b/src/command_helpers.rs @@ -166,7 +166,7 @@ impl StderrForwarder { write_warning(&buffer[..old_data_end]); } - let child_pid = self.inner.take().unwrap().id(); + let id = self.inner.take().unwrap().id(); if let Err(err) = res { write_warning(format!("Failed to read from child {id} stderr: {err}").as_bytes()); } From fb1f249f0f2e066f601cbccb8d0885ad561653dc Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 2 Jun 2024 16:24:11 +1000 Subject: [PATCH 3/5] Fix CI --- src/command_helpers.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/command_helpers.rs b/src/command_helpers.rs index 28fdbbc69..ad3ebf993 100644 --- a/src/command_helpers.rs +++ b/src/command_helpers.rs @@ -138,7 +138,9 @@ impl StderrForwarder { buffer.reserve(to_reserve); // 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(unsafe { &mut *(buffer.spare_capacity_mut() as *mut _ as *mut [u8]) }) + { Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => { // No data currently, yield back. break false; @@ -166,9 +168,11 @@ impl StderrForwarder { write_warning(&buffer[..old_data_end]); } - let id = self.inner.take().unwrap().id(); + let id = self.inner.take().unwrap().0.id(); if let Err(err) = res { - write_warning(format!("Failed to read from child {id} stderr: {err}").as_bytes()); + write_warning( + format!("Failed to read from child {id} stderr: {err}").as_bytes(), + ); } break true; } From 786ecd7b92286c6f09fb1aabc16c47d02726a392 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 2 Jun 2024 16:25:36 +1000 Subject: [PATCH 4/5] Fix compilation --- src/command_helpers.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/command_helpers.rs b/src/command_helpers.rs index ad3ebf993..1dd0a74a9 100644 --- a/src/command_helpers.rs +++ b/src/command_helpers.rs @@ -167,13 +167,12 @@ impl StderrForwarder { if old_data_end > 0 { write_warning(&buffer[..old_data_end]); } - - let id = self.inner.take().unwrap().0.id(); if let Err(err) = res { write_warning( - format!("Failed to read from child {id} stderr: {err}").as_bytes(), + format!("Failed to read from child stderr: {err}").as_bytes(), ); } + self.inner.take(); break true; } } From 8f0e10804b3ab90db2436f696cf0597f3a7768ca Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sat, 8 Jun 2024 08:11:39 +1000 Subject: [PATCH 5/5] Rm unused allow lints --- src/command_helpers.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/command_helpers.rs b/src/command_helpers.rs index 1dd0a74a9..ca1ffaf95 100644 --- a/src/command_helpers.rs +++ b/src/command_helpers.rs @@ -90,7 +90,6 @@ impl StderrForwarder { } } - #[allow(clippy::uninit_vec)] fn forward_available(&mut self) -> bool { if let Some((stderr, buffer)) = self.inner.as_mut() { loop {