From 57169c75f2ebd095defd7392f42b3e916fd2ad2f Mon Sep 17 00:00:00 2001 From: hermes Date: Sat, 17 Oct 2026 13:24:20 +0000 Subject: [PATCH] revert: keep curl in idx's process group so Ctrl+C reaches it Reverts a360d0b. A separate process group let an overflow kill the curl behind a profile script, but Ctrl+C then reached only idx and left a stalled transfer running (Pullfrog review on #55). Cancellation is the common path; on overflow, curl still ends on its next write to the closed pipe, and the error returns at once either way. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01V3syn2fN2LNo5pvTSqc6eh --- Cargo.lock | 1 - crates/idx-core/Cargo.toml | 3 -- crates/idx-core/src/curl_impersonate.rs | 60 +++++-------------------- 3 files changed, 10 insertions(+), 54 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 07b53a3..8de2fe0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -908,7 +908,6 @@ dependencies = [ "clap", "directories", "fastrand", - "libc", "quick-xml", "rusqlite", "serde", diff --git a/crates/idx-core/Cargo.toml b/crates/idx-core/Cargo.toml index 1965fd8..1ac22af 100644 --- a/crates/idx-core/Cargo.toml +++ b/crates/idx-core/Cargo.toml @@ -34,6 +34,3 @@ rusqlite = { workspace = true, optional = true } quick-xml = { version = "0.37", optional = true } sha2 = { workspace = true, optional = true } zip = { workspace = true, optional = true } - -[target.'cfg(unix)'.dependencies] -libc = "0.2" diff --git a/crates/idx-core/src/curl_impersonate.rs b/crates/idx-core/src/curl_impersonate.rs index ac7f512..a157b09 100644 --- a/crates/idx-core/src/curl_impersonate.rs +++ b/crates/idx-core/src/curl_impersonate.rs @@ -118,25 +118,12 @@ fn run_binary_limited( limit: usize, ) -> Result { let max_filesize = limit.to_string(); - let mut command = Command::new(binary); - command - .args([ - "--max-time", - MAX_TIME_SECS, - "--max-filesize", - max_filesize.as_str(), - ]) + let mut child = Command::new(binary) + .args(["--max-time", MAX_TIME_SECS, "--max-filesize", max_filesize.as_str()]) .args(args) .stdin(Stdio::null()) .stdout(Stdio::piped()) - .stderr(Stdio::piped()); - // The curl_* profiles are scripts that run curl without exec; a process - // group of its own lets an overflow kill curl too, not just the script. - // Ctrl+C then reaches only idx: curl ends on its next write to the closed - // pipe, or at --max-time. - #[cfg(unix)] - std::os::unix::process::CommandExt::process_group(&mut command, 0); - let mut child = command + .stderr(Stdio::piped()) .spawn() .map_err(|e| { if e.kind() == std::io::ErrorKind::NotFound { @@ -160,10 +147,14 @@ fn run_binary_limited( .take(limit as u64 + 1) .read_to_end(&mut stdout); if read.is_err() || stdout.len() > limit { + // The curl_* profiles are scripts that run curl without exec, so the + // kill reaches only the wrapper; curl itself still holds stderr. Do + // not wait for it: closing stdout ends it on its next write (or + // --max-time ends a stalled one), and the stderr thread exits with it. drop(stdout_pipe); - kill_process_group(&mut child); + let _ = child.kill(); let _ = child.wait(); - let _ = stderr_reader.join(); + drop(stderr_reader); return Err(match read { Err(e) => IdxError::Http(format!( "failed to read {stage} response from {binary}: {e}" @@ -198,18 +189,6 @@ fn run_binary_limited( ))) } -/// Kills curl and the profile script around it (their group, on Unix). -fn kill_process_group(child: &mut std::process::Child) { - #[cfg(unix)] - if let Ok(pgid) = libc::pid_t::try_from(child.id()) { - // SAFETY: kill(2) takes plain integers; the group is the one this - // child leads (process_group(0) at spawn), not yet reaped. - unsafe { libc::kill(-pgid, libc::SIGKILL) }; - return; - } - let _ = child.kill(); -} - #[cfg(all(test, unix))] mod tests { use super::*; @@ -221,13 +200,9 @@ mod tests { let dir = std::env::temp_dir().join(format!("idx-curl-{}", fastrand::u64(..))); std::fs::create_dir_all(&dir).unwrap(); let fake = dir.join("curl"); - let pid_file = dir.join("curl.pid"); std::fs::write( &fake, - format!( - "#!/bin/sh\nsh -c 'echo $$ > {}; head -c 8192 /dev/zero; exec sleep 30'\ntrue\n", - pid_file.display() - ), + "#!/bin/sh\n( head -c 8192 /dev/zero; sleep 30 )\ntrue\n", ) .unwrap(); let mut perms = std::fs::metadata(&fake).unwrap().permissions(); @@ -239,21 +214,6 @@ mod tests { assert!(err.to_string().contains("exceeds 4096 bytes"), "{err}"); assert!(started.elapsed() < std::time::Duration::from_secs(10)); - // The stand-in curl behind the wrapper is gone too, not left to its - // timeout (allow a moment for init to reap it). - let pid: libc::pid_t = std::fs::read_to_string(&pid_file) - .unwrap() - .trim() - .parse() - .unwrap(); - let alive = || unsafe { libc::kill(pid, 0) } == 0; - for _ in 0..50 { - if !alive() { - break; - } - std::thread::sleep(std::time::Duration::from_millis(100)); - } - assert!(!alive(), "curl {pid} outlived the overflow"); std::fs::remove_dir_all(&dir).unwrap(); } }