mirror of
https://github.com/rustdesk/hbb_common.git
synced 2026-08-27 12:39:50 +00:00
fix: bound the wait and the pipe reads against a group-escaping descendant
Two P3 hardening items from the review: a descendant that changes its own process group escapes the deadline's group kill, and could then leak or block the parent. - The deadline path now also sends a pid-targeted SIGKILL to the direct child, so child.wait() is bounded even if the child left the group and the group kill missed it. - The normal-exit path drains stdout and stderr non-blocking instead of read_to_string: the child has exited so its output is already buffered, but an escaped grandchild holding a write end would keep the pipe from EOF and hang a blocking read. The drain is capped so a descendant that keeps writing cannot spin it. first_buffered_line now shares that drain. Verified: a probe child whose grandchild setpgid-escapes and holds the pipe returns in 25 ms instead of hanging, and a direct child that escapes and blocks is bounded to the deadline instead of its full sleep.
This commit is contained in:
@@ -168,7 +168,6 @@ fn wayland_sockets_in(dir: &Path) -> Vec<PathBuf> {
|
|||||||
pub(super) fn wayland_displays_from_runtime_dir(
|
pub(super) fn wayland_displays_from_runtime_dir(
|
||||||
named_endpoint: bool,
|
named_endpoint: bool,
|
||||||
) -> ResultType<Vec<WaylandDisplayInfo>> {
|
) -> ResultType<Vec<WaylandDisplayInfo>> {
|
||||||
use std::io::Read;
|
|
||||||
use std::sync::atomic::Ordering;
|
use std::sync::atomic::Ordering;
|
||||||
if named_endpoint {
|
if named_endpoint {
|
||||||
bail!("an explicit wayland endpoint is set and did not connect");
|
bail!("an explicit wayland endpoint is set and did not connect");
|
||||||
@@ -204,6 +203,10 @@ pub(super) fn wayland_displays_from_runtime_dir(
|
|||||||
}
|
}
|
||||||
None if std::time::Instant::now() >= deadline => {
|
None if std::time::Instant::now() >= deadline => {
|
||||||
kill_probe_group();
|
kill_probe_group();
|
||||||
|
// The direct pid too, not only its group: if the child left the group its own
|
||||||
|
// kill would miss it, and the wait below would then block on a live child. A
|
||||||
|
// pid-targeted SIGKILL is uncatchable, so wait() is bounded either way.
|
||||||
|
let _ = child.kill();
|
||||||
let _ = child.wait();
|
let _ = child.wait();
|
||||||
// An unwired binary runs its normal startup, and a long-running one (the
|
// An unwired binary runs its normal startup, and a long-running one (the
|
||||||
// server itself) lands HERE rather than at the handshake check below — latch
|
// server itself) lands HERE rather than at the handshake check below — latch
|
||||||
@@ -229,14 +232,11 @@ pub(super) fn wayland_displays_from_runtime_dir(
|
|||||||
None => std::thread::sleep(std::time::Duration::from_millis(25)),
|
None => std::thread::sleep(std::time::Duration::from_millis(25)),
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
let mut stdout = String::new();
|
// Drained non-blocking, not read_to_string: the child exited so its output is already
|
||||||
let mut stderr = String::new();
|
// buffered, but a descendant that escaped the process group could still hold a write end open
|
||||||
if let Some(mut pipe) = child.stdout.take() {
|
// and an EOF-seeking read would then hang here forever.
|
||||||
let _ = pipe.read_to_string(&mut stdout);
|
let stdout = drain_nonblocking(child.stdout.take()).unwrap_or_default();
|
||||||
}
|
let stderr = drain_nonblocking(child.stderr.take()).unwrap_or_default();
|
||||||
if let Some(mut pipe) = child.stderr.take() {
|
|
||||||
let _ = pipe.read_to_string(&mut stderr);
|
|
||||||
}
|
|
||||||
let mut lines = stdout.lines();
|
let mut lines = stdout.lines();
|
||||||
if lines.next() != Some(WAYLAND_PROBE_MAGIC) {
|
if lines.next() != Some(WAYLAND_PROBE_MAGIC) {
|
||||||
// Not a probe: the binary ran its normal startup. Latch, or this path would spawn one
|
// Not a probe: the binary ran its normal startup. Latch, or this path would spawn one
|
||||||
@@ -269,14 +269,12 @@ pub(super) fn wayland_displays_from_runtime_dir(
|
|||||||
Ok(displays)
|
Ok(displays)
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The first line already sitting in the pipe buffer, read strictly non-blocking: children of a
|
/// Everything already buffered in the pipe, read strictly non-blocking and capped: a descendant
|
||||||
/// killed consumer can inherit the write end and keep it open, so an EOF-seeking read here could
|
/// that escaped the probe's process group can hold a write end open, so a blocking read (even
|
||||||
/// hang the enumeration forever. Outer `None` means the pipe could not be INSPECTED (missing
|
/// after the child exits) could hang the enumeration forever. `None` means the pipe could not be
|
||||||
/// handle, fcntl or read failure) and must not be read as evidence of anything; `Some(None)` is
|
/// INSPECTED (missing handle or fcntl failure) and must not be read as evidence of anything;
|
||||||
/// an inspected-and-empty buffer.
|
/// `Some` is whatever bytes were buffered, whether or not EOF arrived.
|
||||||
fn first_buffered_line(pipe: Option<std::process::ChildStdout>) -> Option<Option<String>> {
|
fn drain_nonblocking<R: std::io::Read + std::os::fd::AsRawFd>(pipe: Option<R>) -> Option<String> {
|
||||||
use std::io::Read;
|
|
||||||
use std::os::fd::AsRawFd;
|
|
||||||
let mut pipe = pipe?;
|
let mut pipe = pipe?;
|
||||||
let fd = pipe.as_raw_fd();
|
let fd = pipe.as_raw_fd();
|
||||||
unsafe {
|
unsafe {
|
||||||
@@ -285,23 +283,32 @@ fn first_buffered_line(pipe: Option<std::process::ChildStdout>) -> Option<Option
|
|||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
// The magic line is written in one flush and fits many times over; one read is enough.
|
// Capped so a descendant that keeps writing cannot spin this read forever.
|
||||||
let mut buf = vec![0u8; 256];
|
const CAP: usize = 64 * 1024;
|
||||||
match pipe.read(&mut buf) {
|
let mut out = Vec::new();
|
||||||
Ok(n) => {
|
let mut buf = [0u8; 4096];
|
||||||
buf.truncate(n);
|
loop {
|
||||||
Some(
|
match pipe.read(&mut buf) {
|
||||||
String::from_utf8_lossy(&buf)
|
Ok(0) => break, // EOF: the write end is fully closed
|
||||||
.lines()
|
Ok(n) => {
|
||||||
.next()
|
out.extend_from_slice(&buf[..n]);
|
||||||
.map(str::to_owned),
|
if out.len() >= CAP {
|
||||||
)
|
break;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
Err(err) if err.kind() == std::io::ErrorKind::Interrupted => continue,
|
||||||
|
// WouldBlock: what is buffered is drained (a descendant may still hold the writer).
|
||||||
|
// Any other error: stop with what we have.
|
||||||
|
Err(_) => break,
|
||||||
}
|
}
|
||||||
// A drained pipe answers WouldBlock here, and an empty buffer after a whole deadline IS
|
|
||||||
// evidence; any error still counts as uninspectable.
|
|
||||||
Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => Some(None),
|
|
||||||
Err(_) => None,
|
|
||||||
}
|
}
|
||||||
|
Some(String::from_utf8_lossy(&out).into_owned())
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The first line the child buffered, for the timeout latch decision. `Some(None)` is an
|
||||||
|
/// inspected-but-empty buffer (genuine absence of the handshake); outer `None` is uninspectable.
|
||||||
|
fn first_buffered_line(pipe: Option<std::process::ChildStdout>) -> Option<Option<String>> {
|
||||||
|
drain_nonblocking(pipe).map(|s| s.lines().next().map(str::to_owned))
|
||||||
}
|
}
|
||||||
|
|
||||||
fn probe_runtime_dir(dir: &Path) -> ResultType<Vec<WaylandDisplayInfo>> {
|
fn probe_runtime_dir(dir: &Path) -> ResultType<Vec<WaylandDisplayInfo>> {
|
||||||
|
|||||||
Reference in New Issue
Block a user