diff --git a/src/new/orig.rs b/src/new/orig.rs index 5e61cf0..4d250dd 100644 --- a/src/new/orig.rs +++ b/src/new/orig.rs @@ -12,7 +12,7 @@ //! independently of the upstream sources. use std::ffi::OsString; -use std::io::{Read, Write}; +use std::io::Read; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use std::time::Duration; @@ -225,39 +225,60 @@ fn git_archive_tarball( .into()); } - let file = std::fs::File::create(&dest)?; - let mut encoder = XzEncoder::new(file, 6); - let mut child = Command::new("git") - .args([ - "archive", - "--format=tar", - &format!("--prefix={name}-{upstream_version}/"), - tag, - ]) - .current_dir(repo) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - .map_err(|e| format!("failed to run 'git archive': {e}"))?; - let mut stdout = child - .stdout - .take() - .ok_or_else(|| "git archive produced no output".to_string())?; - std::io::copy(&mut stdout, &mut encoder) - .map_err(|e| format!("cannot pipe git archive into '{}': {e}", dest.display()))?; - encoder - .finish() - .map_err(|e| format!("failed to write '{}': {e}", dest.display()))?; - let status = child.wait()?; - if !status.success() { - // The half-written xz file is not a valid tarball: remove it. - let _ = std::fs::remove_file(&dest); - return Err(format!( - "'git archive --format=tar {}' failed with status: {status} \ - (is HEAD exactly on the tag '{tag}'?)", - tag - ) - .into()); + // Any failure after the destination file was created (spawn error, + // broken pipe, failed git) leaves behind an empty or half-written xz + // file that is not a valid tarball: it is removed before returning. + let outcome = (|| { + let file = std::fs::File::create(&dest)?; + let mut encoder = XzEncoder::new(file, 6); + // git's stderr is inherited, not piped: nothing here drains a piped + // stderr, and a chatty git filling the 64 KiB pipe buffer would + // deadlock the archive — its diagnostics belong on the terminal + // anyway, like every other child process in this module. + let mut child = Command::new("git") + .args([ + "archive", + "--format=tar", + &format!("--prefix={name}-{upstream_version}/"), + tag, + ]) + .current_dir(repo) + .stdout(Stdio::piped()) + .spawn() + .map_err(|e| format!("failed to run 'git archive': {e}"))?; + let mut stdout = child + .stdout + .take() + .ok_or_else(|| "git archive produced no output".to_string())?; + let copied = std::io::copy(&mut stdout, &mut encoder) + .map_err(|e| format!("cannot pipe git archive into '{}': {e}", dest.display())); + // The child is reaped whatever happened to the pipe, and a copy + // error only surfaces once it has been waited on. Closing our end + // first unblocks a git still writing into the pipe (SIGPIPE); + // otherwise `wait` could hang on it forever. + drop(stdout); + let status = child.wait()?; + copied?; + encoder + .finish() + .map_err(|e| format!("failed to write '{}': {e}", dest.display()))?; + Ok(status) + })(); + match outcome { + Ok(status) if status.success() => {} + Ok(status) => { + let _ = std::fs::remove_file(&dest); + return Err(format!( + "'git archive --format=tar {}' failed with status: {status} \ + (is HEAD exactly on the tag '{tag}'?)", + tag + ) + .into()); + } + Err(error) => { + let _ = std::fs::remove_file(&dest); + return Err(error); + } } log::info!( @@ -340,28 +361,44 @@ fn fetch_and_repack( /// blocking client must not run on a tokio worker thread (scaffold is /// called from inside the async runtime), so the download runs on a plain /// dedicated thread. +/// +/// Only the connection carries a timeout (10 s, like the rest of the +/// codebase): a whole-request timeout would cap the ENTIRE download and +/// always fail large release tarballs on slow links. The body is instead +/// streamed to the temporary file chunk by chunk as it arrives, never +/// buffered whole in memory. fn download_to_temp(url: &str) -> Result> { let url = url.to_string(); - let contents = std::thread::spawn(move || { - reqwest::blocking::Client::builder() - .timeout(Duration::from_secs(30)) + let download = move || -> Result> { + let mut response = reqwest::blocking::Client::builder() + .connect_timeout(Duration::from_secs(10)) .build()? .get(&url) .send()? - .error_for_status()? - .bytes() - }) - .join() - .map_err(|_| -> Box { "the download thread panicked".into() })??; - - let temp = std::env::temp_dir().join(format!( - "pkh-orig-{}-{}", - std::process::id(), - chrono::Utc::now().timestamp_nanos_opt().unwrap_or_default() - )); - let mut file = std::fs::File::create(&temp)?; - file.write_all(&contents)?; - Ok(temp) + .error_for_status()?; + let temp = std::env::temp_dir().join(format!( + "pkh-orig-{}-{}", + std::process::id(), + chrono::Utc::now().timestamp_nanos_opt().unwrap_or_default() + )); + let mut file = std::fs::File::create(&temp)?; + if let Err(error) = response.copy_to(&mut file) { + // A failed download must not leave a partial temporary behind. + let _ = std::fs::remove_file(&temp); + return Err(error.into()); + } + Ok(temp) + }; + let downloaded: Result> = + std::thread::spawn(download) + .join() + .map_err(|_| -> Box { "the download thread panicked".into() })?; + // The thread must carry a `Send + Sync` error box; the auto traits are + // dropped on the way out. + match downloaded { + Ok(temp) => Ok(temp), + Err(error) => Err(error), + } } /// Container/compression format of a tarball, detected from its content @@ -508,17 +545,24 @@ fn repack_tarball_file( }; let result = repack_tar_stream(reader, name, upstream_version, &dest); - if let Some(mut child) = bzip2_child - && let Ok(dest_path) = &result - { - let status = child.wait()?; - if !status.success() { - let _ = std::fs::remove_file(dest_path); - return Err(format!( - "'bzip2 -dc {}' failed with status: {status}", - path.display() - ) - .into()); + if let Some(mut child) = bzip2_child { + if result.is_err() { + // The repack failed and nothing reads the child's stdout + // anymore: kill it (harmless if it already exited — bzip2 may + // be blocked writing into the unread pipe) and reap it, + // instead of leaking a zombie holding an open pipe. + let _ = child.kill(); + let _ = child.wait(); + } else { + let status = child.wait()?; + if !status.success() { + let _ = std::fs::remove_file(&dest); + return Err(format!( + "'bzip2 -dc {}' failed with status: {status}", + path.display() + ) + .into()); + } } } if result.is_err() { @@ -703,6 +747,7 @@ fn repack_tar_stream( #[cfg(test)] mod tests { use super::*; + use std::io::Write; /// List the entry names of an xz tarball. fn tarball_names(path: &Path) -> Vec {