mirror of
https://github.com/0xrsydn/idx-cli.git
synced 2026-11-07 09:45:36 +00:00
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V3syn2fN2LNo5pvTSqc6eh
This commit is contained in:
parent
a360d0b429
commit
57169c75f2
3 changed files with 10 additions and 54 deletions
1
Cargo.lock
generated
1
Cargo.lock
generated
|
|
@ -908,7 +908,6 @@ dependencies = [
|
||||||
"clap",
|
"clap",
|
||||||
"directories",
|
"directories",
|
||||||
"fastrand",
|
"fastrand",
|
||||||
"libc",
|
|
||||||
"quick-xml",
|
"quick-xml",
|
||||||
"rusqlite",
|
"rusqlite",
|
||||||
"serde",
|
"serde",
|
||||||
|
|
|
||||||
|
|
@ -34,6 +34,3 @@ rusqlite = { workspace = true, optional = true }
|
||||||
quick-xml = { version = "0.37", optional = true }
|
quick-xml = { version = "0.37", optional = true }
|
||||||
sha2 = { workspace = true, optional = true }
|
sha2 = { workspace = true, optional = true }
|
||||||
zip = { workspace = true, optional = true }
|
zip = { workspace = true, optional = true }
|
||||||
|
|
||||||
[target.'cfg(unix)'.dependencies]
|
|
||||||
libc = "0.2"
|
|
||||||
|
|
|
||||||
|
|
@ -118,25 +118,12 @@ fn run_binary_limited(
|
||||||
limit: usize,
|
limit: usize,
|
||||||
) -> Result<Output, IdxError> {
|
) -> Result<Output, IdxError> {
|
||||||
let max_filesize = limit.to_string();
|
let max_filesize = limit.to_string();
|
||||||
let mut command = Command::new(binary);
|
let mut child = Command::new(binary)
|
||||||
command
|
.args(["--max-time", MAX_TIME_SECS, "--max-filesize", max_filesize.as_str()])
|
||||||
.args([
|
|
||||||
"--max-time",
|
|
||||||
MAX_TIME_SECS,
|
|
||||||
"--max-filesize",
|
|
||||||
max_filesize.as_str(),
|
|
||||||
])
|
|
||||||
.args(args)
|
.args(args)
|
||||||
.stdin(Stdio::null())
|
.stdin(Stdio::null())
|
||||||
.stdout(Stdio::piped())
|
.stdout(Stdio::piped())
|
||||||
.stderr(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
|
|
||||||
.spawn()
|
.spawn()
|
||||||
.map_err(|e| {
|
.map_err(|e| {
|
||||||
if e.kind() == std::io::ErrorKind::NotFound {
|
if e.kind() == std::io::ErrorKind::NotFound {
|
||||||
|
|
@ -160,10 +147,14 @@ fn run_binary_limited(
|
||||||
.take(limit as u64 + 1)
|
.take(limit as u64 + 1)
|
||||||
.read_to_end(&mut stdout);
|
.read_to_end(&mut stdout);
|
||||||
if read.is_err() || stdout.len() > limit {
|
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);
|
drop(stdout_pipe);
|
||||||
kill_process_group(&mut child);
|
let _ = child.kill();
|
||||||
let _ = child.wait();
|
let _ = child.wait();
|
||||||
let _ = stderr_reader.join();
|
drop(stderr_reader);
|
||||||
return Err(match read {
|
return Err(match read {
|
||||||
Err(e) => IdxError::Http(format!(
|
Err(e) => IdxError::Http(format!(
|
||||||
"failed to read {stage} response from {binary}: {e}"
|
"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))]
|
#[cfg(all(test, unix))]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
|
|
@ -221,13 +200,9 @@ mod tests {
|
||||||
let dir = std::env::temp_dir().join(format!("idx-curl-{}", fastrand::u64(..)));
|
let dir = std::env::temp_dir().join(format!("idx-curl-{}", fastrand::u64(..)));
|
||||||
std::fs::create_dir_all(&dir).unwrap();
|
std::fs::create_dir_all(&dir).unwrap();
|
||||||
let fake = dir.join("curl");
|
let fake = dir.join("curl");
|
||||||
let pid_file = dir.join("curl.pid");
|
|
||||||
std::fs::write(
|
std::fs::write(
|
||||||
&fake,
|
&fake,
|
||||||
format!(
|
"#!/bin/sh\n( head -c 8192 /dev/zero; sleep 30 )\ntrue\n",
|
||||||
"#!/bin/sh\nsh -c 'echo $$ > {}; head -c 8192 /dev/zero; exec sleep 30'\ntrue\n",
|
|
||||||
pid_file.display()
|
|
||||||
),
|
|
||||||
)
|
)
|
||||||
.unwrap();
|
.unwrap();
|
||||||
let mut perms = std::fs::metadata(&fake).unwrap().permissions();
|
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!(err.to_string().contains("exceeds 4096 bytes"), "{err}");
|
||||||
assert!(started.elapsed() < std::time::Duration::from_secs(10));
|
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();
|
std::fs::remove_dir_all(&dir).unwrap();
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue