From 3158034de9dfdb203472cf224973b57ec61579d9 Mon Sep 17 00:00:00 2001 From: sdairs Date: Wed, 26 Aug 2026 14:31:49 +0100 Subject: [PATCH 1/2] Make local install failures actionable --- crates/clickhousectl/src/error.rs | 24 +- crates/clickhousectl/src/local/cli.rs | 64 +++- crates/clickhousectl/src/paths.rs | 41 ++- .../src/version_manager/install.rs | 312 ++++++++++++++++-- 4 files changed, 401 insertions(+), 40 deletions(-) diff --git a/crates/clickhousectl/src/error.rs b/crates/clickhousectl/src/error.rs index 10df5f1b..b92df06d 100644 --- a/crates/clickhousectl/src/error.rs +++ b/crates/clickhousectl/src/error.rs @@ -33,8 +33,12 @@ pub enum Error { #[error("Unsupported platform: {os}/{arch}")] UnsupportedPlatform { os: String, arch: String }, - #[error("Failed to create directory: {0}")] - CreateDir(PathBuf), + #[error("Failed to create directory '{}': {source}", path.display())] + CreateDir { + path: PathBuf, + #[source] + source: std::io::Error, + }, #[error("Download failed: {0}")] Download(String), @@ -180,4 +184,20 @@ mod tests { assert_eq!(Error::Cancelled.exit_code(), 3); assert_eq!(Error::AuthRequired("nope".into()).exit_code(), 4); } + + #[test] + fn create_dir_error_includes_path_and_permission_cause() { + let error = Error::CreateDir { + path: PathBuf::from("/read-only/clickhouse/versions"), + source: std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "permission denied by test", + ), + }; + + assert_eq!( + error.to_string(), + "Failed to create directory '/read-only/clickhouse/versions': permission denied by test" + ); + } } diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index 2da4b489..df9edcd3 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -2,6 +2,25 @@ use crate::version_manager::{self, VersionSpec}; use clap::{Args, Subcommand}; use std::str::FromStr; +const INSTALL_AFTER_HELP: &str = "\ +EXAMPLES: + clickhousectl local install latest + clickhousectl local install 26.8 + clickhousectl local install 26.8.1.1760 + clickhousectl local install postgres@18 + +CLICKHOUSE DOWNLOAD: + Installs each binary at ~/.clickhouse/versions//clickhouse. + Expect an approximately 150 MB download from builds.clickhouse.com, with fallback downloads from + packages.clickhouse.com on Linux or github.com on macOS. + +BOOTSTRAP ALTERNATIVE: + `clickhousectl local server start` needs no separate install: it installs `latest` when needed and + starts a server without setting a default version. + +CONTEXT FOR AGENTS: + `clickhousectl local use ` will auto-install if the version is missing and set as default."; + #[derive(Debug, Clone, PartialEq, Eq)] pub enum InstallVersionArg { ClickHouse(VersionSpec), @@ -95,9 +114,7 @@ pub struct LocalArgs { #[derive(Subcommand)] pub enum LocalCommands { /// Install a ClickHouse version - #[command(after_help = "\ -CONTEXT FOR AGENTS: - `clickhousectl local use ` will auto-install if the version is missing and set as default.")] + #[command(after_help = INSTALL_AFTER_HELP)] Install { /// Version to install. Accepts: "latest" (recommended), "stable", "lts", partial like "25.12", exact like "25.12.9.61", or a Postgres image selector like "postgres@18". version: InstallVersionArg, @@ -666,6 +683,47 @@ mod tests { ); } + #[test] + fn install_help_is_complete_and_stable() { + let error = Cli::try_parse_from(["clickhousectl", "local", "install", "--help"]) + .err() + .expect("--help should stop parsing"); + assert_eq!(error.kind(), clap::error::ErrorKind::DisplayHelp); + assert_eq!( + error.to_string(), + r#"Install a ClickHouse version + +Usage: clickhousectl local install [OPTIONS] + +Arguments: + Version to install. Accepts: "latest" (recommended), "stable", "lts", partial like "25.12", exact like "25.12.9.61", or a Postgres image selector like "postgres@18" + +Options: + --force Force re-install even if version is already installed + --json Output as JSON + -h, --help Print help + +EXAMPLES: + clickhousectl local install latest + clickhousectl local install 26.8 + clickhousectl local install 26.8.1.1760 + clickhousectl local install postgres@18 + +CLICKHOUSE DOWNLOAD: + Installs each binary at ~/.clickhouse/versions//clickhouse. + Expect an approximately 150 MB download from builds.clickhouse.com, with fallback downloads from + packages.clickhouse.com on Linux or github.com on macOS. + +BOOTSTRAP ALTERNATIVE: + `clickhousectl local server start` needs no separate install: it installs `latest` when needed and + starts a server without setting a default version. + +CONTEXT FOR AGENTS: + `clickhousectl local use ` will auto-install if the version is missing and set as default. +"# + ); + } + #[test] fn use_help_documents_standard_clickhouse_subcommands() { let error = Cli::try_parse_from(["clickhousectl", "local", "use", "--help"]) diff --git a/crates/clickhousectl/src/paths.rs b/crates/clickhousectl/src/paths.rs index 2375ad6a..50e3b032 100644 --- a/crates/clickhousectl/src/paths.rs +++ b/crates/clickhousectl/src/paths.rs @@ -1,5 +1,5 @@ use crate::error::{Error, Result}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; /// Returns the base directory for ClickHouse CLI (~/.clickhouse/) pub fn base_dir() -> Result { @@ -40,8 +40,14 @@ pub fn configs_dir() -> Result { /// Ensures all necessary directories exist pub fn ensure_dirs() -> Result<()> { let versions = versions_dir()?; - std::fs::create_dir_all(&versions).map_err(|_| Error::CreateDir(versions))?; - Ok(()) + ensure_dir(&versions) +} + +pub(crate) fn ensure_dir(path: &Path) -> Result<()> { + std::fs::create_dir_all(path).map_err(|source| Error::CreateDir { + path: path.to_path_buf(), + source, + }) } /// Returns the user-local PATH-style bin directory (~/.local/bin/) @@ -59,3 +65,32 @@ pub fn global_bin_dir() -> Result { pub fn global_clickhouse_symlink() -> Result { Ok(global_bin_dir()?.join("clickhouse")) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn ensure_dir_includes_path_and_not_a_directory_cause() { + let temp = tempfile::tempdir().unwrap(); + let file = temp.path().join("not-a-directory"); + std::fs::write(&file, "content").unwrap(); + let directory = file.join("versions"); + + let error = ensure_dir(&directory).unwrap_err(); + let Error::CreateDir { path, source } = &error else { + panic!("expected create-directory error: {error}"); + }; + + assert_eq!(path, &directory); + assert_eq!(source.kind(), std::io::ErrorKind::NotADirectory); + assert_eq!( + error.to_string(), + format!( + "Failed to create directory '{}': {}", + directory.display(), + source + ) + ); + } +} diff --git a/crates/clickhousectl/src/version_manager/install.rs b/crates/clickhousectl/src/version_manager/install.rs index bce6b123..b761e62c 100644 --- a/crates/clickhousectl/src/version_manager/install.rs +++ b/crates/clickhousectl/src/version_manager/install.rs @@ -6,7 +6,13 @@ use crate::version_manager::master; use crate::version_manager::platform::{DownloadSource, Platform}; use crate::version_manager::resolve::{ResolvedVersion, resolve, try_resolve_local}; use crate::version_manager::spec::VersionSpec; +use flate2::read::GzDecoder; +use std::ffi::OsStr; +use std::fs::File; +use std::io::{self, Write}; use std::os::unix::fs::PermissionsExt; +use std::path::{Component, Path}; +use tar::Archive; /// Install a version spec, trying installed versions first before any remote call. /// An installed match is a successful no-op regardless of whether the spec is @@ -103,7 +109,7 @@ pub async fn install_resolved( if temp_dir.exists() { std::fs::remove_dir_all(&temp_dir)?; } - std::fs::create_dir_all(&temp_dir)?; + paths::ensure_dir(&temp_dir)?; let binary_path = temp_dir.join("clickhouse"); @@ -147,7 +153,7 @@ pub async fn install_resolved( if replaced_existing { std::fs::remove_dir_all(&version_dir)?; } - std::fs::create_dir_all(&version_dir)?; + paths::ensure_dir(&version_dir)?; std::fs::rename(&binary_path, version_dir.join("clickhouse"))?; // Replacing a build on disk never affects already-running servers (they keep @@ -271,49 +277,184 @@ fn parse_version_output(output: &str) -> Result { /// Handles both packages.clickhouse.com layout (usr/bin/clickhouse inside subdir) /// and GitHub releases layout (same structure). fn extract_tarball_auto(tarball_path: &std::path::Path, dest_dir: &std::path::Path) -> Result<()> { - let status = std::process::Command::new("tar") - .args(["xzf", &tarball_path.to_string_lossy()]) - .current_dir(dest_dir) - .status() - .map_err(|e| Error::Extract(format!("Failed to run tar: {}", e)))?; - - if !status.success() { - let _ = std::fs::remove_file(tarball_path); - return Err(Error::Extract("tar extraction failed".to_string())); - } - + let archive_file = File::open(tarball_path).map_err(|source| { + Error::Extract(format!( + "Failed to open archive '{}': {}", + tarball_path.display(), + source + )) + })?; + let decoder = GzDecoder::new(archive_file); + let mut archive = Archive::new(decoder); let final_binary = dest_dir.join("clickhouse"); - - // Search extracted directories for the binary - for entry in std::fs::read_dir(dest_dir)? { - let entry = entry?; - let path = entry.path(); - if path.is_dir() { - let candidate = path.join("usr/bin/clickhouse"); - if candidate.exists() { - std::fs::rename(&candidate, &final_binary)?; - let _ = std::fs::remove_file(tarball_path); - let _ = std::fs::remove_dir_all(&path); - return Ok(()); + let partial_binary = dest_dir.join(".clickhouse.extracting"); + + let extraction_result = (|| -> Result<()> { + let entries = archive.entries().map_err(|source| { + Error::Extract(format!( + "Failed to read archive '{}': {}", + tarball_path.display(), + source + )) + })?; + let mut found_binary = false; + + for entry in entries { + let mut entry = entry.map_err(|source| { + Error::Extract(format!( + "Failed to read an entry from archive '{}': {}", + tarball_path.display(), + source + )) + })?; + let entry_path = entry + .path() + .map_err(|source| { + Error::Extract(format!( + "Failed to read an entry path from archive '{}': {}", + tarball_path.display(), + source + )) + })? + .into_owned(); + + validate_archive_entry_path(tarball_path, &entry_path)?; + if !is_clickhouse_binary_path(&entry_path) { + continue; } + if found_binary { + return Err(Error::Extract(format!( + "Archive '{}' contains more than one ClickHouse binary", + tarball_path.display() + ))); + } + if !entry.header().entry_type().is_file() { + return Err(Error::Extract(format!( + "Archive '{}' entry '{}' is not a regular file; symbolic and hard links are not followed", + tarball_path.display(), + entry_path.display() + ))); + } + + let mut output = File::create(&partial_binary).map_err(|source| { + Error::Extract(format!( + "Failed to create extraction file '{}' for archive '{}': {}", + partial_binary.display(), + tarball_path.display(), + source + )) + })?; + io::copy(&mut entry, &mut output) + .and_then(|_| output.flush()) + .map_err(|source| { + Error::Extract(format!( + "Failed to extract '{}' from archive '{}' to '{}': {}", + entry_path.display(), + tarball_path.display(), + partial_binary.display(), + source + )) + })?; + found_binary = true; } + + if !found_binary { + return Err(Error::Extract(format!( + "Archive '{}' does not contain a ClickHouse binary at 'clickhouse' or '*/usr/bin/clickhouse'", + tarball_path.display() + ))); + } + + Ok(()) + })(); + + if let Err(error) = extraction_result { + let _ = std::fs::remove_file(&partial_binary); + return Err(error); } - // Binary might already be at top level - if final_binary.exists() { - let _ = std::fs::remove_file(tarball_path); + if let Err(source) = std::fs::rename(&partial_binary, &final_binary) { + let _ = std::fs::remove_file(&partial_binary); + return Err(Error::Extract(format!( + "Failed to move extracted ClickHouse binary from '{}' to '{}': {}", + partial_binary.display(), + final_binary.display(), + source + ))); + } + let _ = std::fs::remove_file(tarball_path); + Ok(()) +} + +fn validate_archive_entry_path(archive_path: &Path, entry_path: &Path) -> Result<()> { + let safe = !entry_path.as_os_str().is_empty() + && entry_path + .components() + .all(|component| matches!(component, Component::CurDir | Component::Normal(_))); + if safe { return Ok(()); } - let _ = std::fs::remove_file(tarball_path); - Err(Error::Extract( - "Could not find clickhouse binary in extracted tarball".to_string(), - )) + Err(Error::Extract(format!( + "Archive '{}' contains unsafe entry path '{}'", + archive_path.display(), + entry_path.display() + ))) +} + +fn is_clickhouse_binary_path(path: &Path) -> bool { + let components = path + .components() + .filter_map(|component| match component { + Component::Normal(part) => Some(part), + _ => None, + }) + .collect::>(); + + (components.len() == 1 && components[0] == OsStr::new("clickhouse")) + || (components.len() >= 3 + && components[components.len() - 3] == OsStr::new("usr") + && components[components.len() - 2] == OsStr::new("bin") + && components[components.len() - 1] == OsStr::new("clickhouse")) } #[cfg(test)] mod tests { use super::*; + use flate2::{Compression, write::GzEncoder}; + use std::fs; + use tar::{Builder, EntryType, Header}; + + fn write_archive(archive_path: &Path, entry_path: &str, contents: &[u8]) { + let archive_file = File::create(archive_path).unwrap(); + let encoder = GzEncoder::new(archive_file, Compression::default()); + let mut archive = Builder::new(encoder); + let mut header = Header::new_gnu(); + header.set_path(entry_path).unwrap(); + header.set_entry_type(EntryType::Regular); + header.set_mode(0o755); + header.set_size(contents.len() as u64); + header.set_cksum(); + archive.append(&header, contents).unwrap(); + let encoder = archive.into_inner().unwrap(); + encoder.finish().unwrap(); + } + + fn write_symlink_archive(archive_path: &Path, entry_path: &str) { + let archive_file = File::create(archive_path).unwrap(); + let encoder = GzEncoder::new(archive_file, Compression::default()); + let mut archive = Builder::new(encoder); + let mut header = Header::new_gnu(); + header.set_path(entry_path).unwrap(); + header.set_entry_type(EntryType::Symlink); + header.set_link_name("../../outside").unwrap(); + header.set_mode(0o777); + header.set_size(0); + header.set_cksum(); + archive.append(&header, io::empty()).unwrap(); + let encoder = archive.into_inner().unwrap(); + encoder.finish().unwrap(); + } #[test] fn test_parse_version_output_client() { @@ -343,4 +484,111 @@ mod tests { fn test_parse_version_output_empty() { assert!(parse_version_output("").is_err()); } + + #[test] + fn extracts_package_binary_without_host_tar() { + let temp = tempfile::tempdir().unwrap(); + let archive_path = temp.path().join("clickhouse.tgz"); + write_archive( + &archive_path, + "clickhouse-common-static/usr/bin/clickhouse", + b"clickhouse binary", + ); + + extract_tarball_auto(&archive_path, temp.path()).unwrap(); + + assert_eq!( + fs::read(temp.path().join("clickhouse")).unwrap(), + b"clickhouse binary" + ); + assert!(!archive_path.exists()); + assert!(!temp.path().join(".clickhouse.extracting").exists()); + } + + #[test] + fn malformed_archive_error_includes_archive_path_and_cause() { + let temp = tempfile::tempdir().unwrap(); + let archive_path = temp.path().join("broken.tgz"); + fs::write(&archive_path, "not a gzip archive").unwrap(); + + let error = extract_tarball_auto(&archive_path, temp.path()).unwrap_err(); + let message = error.to_string(); + + assert!( + message.contains(&archive_path.display().to_string()), + "{message}" + ); + assert!(message.contains("invalid gzip header"), "{message}"); + assert_ne!(message, "Extraction failed: tar extraction failed"); + assert!(!temp.path().join("clickhouse").exists()); + } + + #[test] + fn missing_binary_error_names_archive_and_expected_paths() { + let temp = tempfile::tempdir().unwrap(); + let archive_path = temp.path().join("missing.tgz"); + write_archive(&archive_path, "package/README.md", b"read me"); + + let error = extract_tarball_auto(&archive_path, temp.path()).unwrap_err(); + + assert_eq!( + error.to_string(), + format!( + "Extraction failed: Archive '{}' does not contain a ClickHouse binary at 'clickhouse' or '*/usr/bin/clickhouse'", + archive_path.display() + ) + ); + } + + #[test] + fn extraction_destination_error_includes_path_and_os_cause() { + let temp = tempfile::tempdir().unwrap(); + let archive_path = temp.path().join("clickhouse.tgz"); + write_archive(&archive_path, "package/usr/bin/clickhouse", b"binary"); + let destination = temp.path().join("not-a-directory"); + fs::write(&destination, "content").unwrap(); + + let error = extract_tarball_auto(&archive_path, &destination).unwrap_err(); + let message = error.to_string(); + + assert!( + message.contains( + &destination + .join(".clickhouse.extracting") + .display() + .to_string() + ), + "{message}" + ); + assert!(message.contains("Not a directory"), "{message}"); + } + + #[test] + fn refuses_link_for_clickhouse_binary() { + let temp = tempfile::tempdir().unwrap(); + let archive_path = temp.path().join("linked.tgz"); + write_symlink_archive(&archive_path, "package/usr/bin/clickhouse"); + + let error = extract_tarball_auto(&archive_path, temp.path()).unwrap_err(); + + assert!( + error + .to_string() + .contains("is not a regular file; symbolic and hard links are not followed") + ); + assert!(!temp.path().join("clickhouse").exists()); + } + + #[test] + fn refuses_archive_path_traversal() { + let archive_path = Path::new("/tmp/clickhouse.tgz"); + let error = + validate_archive_entry_path(archive_path, Path::new("../package/usr/bin/clickhouse")) + .unwrap_err(); + + assert_eq!( + error.to_string(), + "Extraction failed: Archive '/tmp/clickhouse.tgz' contains unsafe entry path '../package/usr/bin/clickhouse'" + ); + } } From d41af8c7acbe700950ed0b9365dc367857c7c2e4 Mon Sep 17 00:00:00 2001 From: sdairs Date: Wed, 26 Aug 2026 21:33:41 +0100 Subject: [PATCH 2/2] Validate local install archives --- crates/clickhousectl/src/error.rs | 12 ++ crates/clickhousectl/src/local/cli.rs | 66 +++------ .../src/version_manager/install.rs | 134 ++++++++++-------- 3 files changed, 107 insertions(+), 105 deletions(-) diff --git a/crates/clickhousectl/src/error.rs b/crates/clickhousectl/src/error.rs index b92df06d..e157a1e1 100644 --- a/crates/clickhousectl/src/error.rs +++ b/crates/clickhousectl/src/error.rs @@ -72,6 +72,18 @@ pub enum Error { #[error("Extraction failed: {0}")] Extract(String), + #[error( + "Failed to extract archive '{}' to '{}': {source}", + archive.display(), + destination.display() + )] + ExtractArchive { + archive: PathBuf, + destination: PathBuf, + #[source] + source: std::io::Error, + }, + #[error("Server '{0}' is not running")] ServerNotRunning(String), diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index df9edcd3..7a79c21d 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -3,6 +3,9 @@ use clap::{Args, Subcommand}; use std::str::FromStr; const INSTALL_AFTER_HELP: &str = "\ +CONTEXT FOR AGENTS: + `clickhousectl local use ` auto-installs a missing version and sets it as default. + EXAMPLES: clickhousectl local install latest clickhousectl local install 26.8 @@ -10,16 +13,10 @@ EXAMPLES: clickhousectl local install postgres@18 CLICKHOUSE DOWNLOAD: - Installs each binary at ~/.clickhouse/versions//clickhouse. - Expect an approximately 150 MB download from builds.clickhouse.com, with fallback downloads from - packages.clickhouse.com on Linux or github.com on macOS. - -BOOTSTRAP ALTERNATIVE: - `clickhousectl local server start` needs no separate install: it installs `latest` when needed and - starts a server without setting a default version. - -CONTEXT FOR AGENTS: - `clickhousectl local use ` will auto-install if the version is missing and set as default."; + Binaries install at ~/.clickhouse/versions//clickhouse and are approximately 150 MB. + Downloads use builds.clickhouse.com, with packages.clickhouse.com fallback on Linux and + github.com on macOS. To bootstrap without setting a default, run + `clickhousectl local server start`; it installs `latest` when needed."; #[derive(Debug, Clone, PartialEq, Eq)] pub enum InstallVersionArg { @@ -684,44 +681,27 @@ mod tests { } #[test] - fn install_help_is_complete_and_stable() { + fn install_help_covers_install_requirements() { let error = Cli::try_parse_from(["clickhousectl", "local", "install", "--help"]) .err() .expect("--help should stop parsing"); assert_eq!(error.kind(), clap::error::ErrorKind::DisplayHelp); - assert_eq!( - error.to_string(), - r#"Install a ClickHouse version - -Usage: clickhousectl local install [OPTIONS] - -Arguments: - Version to install. Accepts: "latest" (recommended), "stable", "lts", partial like "25.12", exact like "25.12.9.61", or a Postgres image selector like "postgres@18" - -Options: - --force Force re-install even if version is already installed - --json Output as JSON - -h, --help Print help - -EXAMPLES: - clickhousectl local install latest - clickhousectl local install 26.8 - clickhousectl local install 26.8.1.1760 - clickhousectl local install postgres@18 - -CLICKHOUSE DOWNLOAD: - Installs each binary at ~/.clickhouse/versions//clickhouse. - Expect an approximately 150 MB download from builds.clickhouse.com, with fallback downloads from - packages.clickhouse.com on Linux or github.com on macOS. - -BOOTSTRAP ALTERNATIVE: - `clickhousectl local server start` needs no separate install: it installs `latest` when needed and - starts a server without setting a default version. + let help = error.to_string(); -CONTEXT FOR AGENTS: - `clickhousectl local use ` will auto-install if the version is missing and set as default. -"# - ); + for required in [ + "clickhousectl local install latest", + "~/.clickhouse/versions//clickhouse", + "approximately 150 MB", + "builds.clickhouse.com", + "packages.clickhouse.com", + "github.com", + "clickhousectl local server start", + ] { + assert!( + help.contains(required), + "missing {required:?} from:\n{help}" + ); + } } #[test] diff --git a/crates/clickhousectl/src/version_manager/install.rs b/crates/clickhousectl/src/version_manager/install.rs index b761e62c..c29b284c 100644 --- a/crates/clickhousectl/src/version_manager/install.rs +++ b/crates/clickhousectl/src/version_manager/install.rs @@ -277,45 +277,29 @@ fn parse_version_output(output: &str) -> Result { /// Handles both packages.clickhouse.com layout (usr/bin/clickhouse inside subdir) /// and GitHub releases layout (same structure). fn extract_tarball_auto(tarball_path: &std::path::Path, dest_dir: &std::path::Path) -> Result<()> { - let archive_file = File::open(tarball_path).map_err(|source| { - Error::Extract(format!( - "Failed to open archive '{}': {}", - tarball_path.display(), - source - )) - })?; - let decoder = GzDecoder::new(archive_file); - let mut archive = Archive::new(decoder); let final_binary = dest_dir.join("clickhouse"); let partial_binary = dest_dir.join(".clickhouse.extracting"); + let extraction_error = |destination: &Path, source| Error::ExtractArchive { + archive: tarball_path.to_path_buf(), + destination: destination.to_path_buf(), + source, + }; + let archive_file = + File::open(tarball_path).map_err(|source| extraction_error(&final_binary, source))?; + let decoder = GzDecoder::new(archive_file); + let mut archive = Archive::new(decoder); let extraction_result = (|| -> Result<()> { - let entries = archive.entries().map_err(|source| { - Error::Extract(format!( - "Failed to read archive '{}': {}", - tarball_path.display(), - source - )) - })?; + let entries = archive + .entries() + .map_err(|source| extraction_error(&final_binary, source))?; let mut found_binary = false; for entry in entries { - let mut entry = entry.map_err(|source| { - Error::Extract(format!( - "Failed to read an entry from archive '{}': {}", - tarball_path.display(), - source - )) - })?; + let mut entry = entry.map_err(|source| extraction_error(&final_binary, source))?; let entry_path = entry .path() - .map_err(|source| { - Error::Extract(format!( - "Failed to read an entry path from archive '{}': {}", - tarball_path.display(), - source - )) - })? + .map_err(|source| extraction_error(&final_binary, source))? .into_owned(); validate_archive_entry_path(tarball_path, &entry_path)?; @@ -336,25 +320,11 @@ fn extract_tarball_auto(tarball_path: &std::path::Path, dest_dir: &std::path::Pa ))); } - let mut output = File::create(&partial_binary).map_err(|source| { - Error::Extract(format!( - "Failed to create extraction file '{}' for archive '{}': {}", - partial_binary.display(), - tarball_path.display(), - source - )) - })?; + let mut output = File::create(&partial_binary) + .map_err(|source| extraction_error(&partial_binary, source))?; io::copy(&mut entry, &mut output) .and_then(|_| output.flush()) - .map_err(|source| { - Error::Extract(format!( - "Failed to extract '{}' from archive '{}' to '{}': {}", - entry_path.display(), - tarball_path.display(), - partial_binary.display(), - source - )) - })?; + .map_err(|source| extraction_error(&partial_binary, source))?; found_binary = true; } @@ -365,6 +335,12 @@ fn extract_tarball_auto(tarball_path: &std::path::Path, dest_dir: &std::path::Pa ))); } + // tar stops at its end-of-archive blocks. Read the gzip stream itself + // to EOF so GzDecoder validates the trailer CRC and uncompressed size. + let mut decoder = archive.into_inner(); + io::copy(&mut decoder, &mut io::sink()) + .map_err(|source| extraction_error(&final_binary, source))?; + Ok(()) })(); @@ -375,12 +351,7 @@ fn extract_tarball_auto(tarball_path: &std::path::Path, dest_dir: &std::path::Pa if let Err(source) = std::fs::rename(&partial_binary, &final_binary) { let _ = std::fs::remove_file(&partial_binary); - return Err(Error::Extract(format!( - "Failed to move extracted ClickHouse binary from '{}' to '{}': {}", - partial_binary.display(), - final_binary.display(), - source - ))); + return Err(extraction_error(&final_binary, source)); } let _ = std::fs::remove_file(tarball_path); Ok(()) @@ -456,6 +427,19 @@ mod tests { encoder.finish().unwrap(); } + fn assert_invalid_gzip_is_not_committed(mutate: impl FnOnce(&Path)) { + let temp = tempfile::tempdir().unwrap(); + let archive_path = temp.path().join("clickhouse.tgz"); + write_archive(&archive_path, "clickhouse", b"clickhouse binary"); + mutate(&archive_path); + + let error = extract_tarball_auto(&archive_path, temp.path()).unwrap_err(); + + assert!(matches!(error, Error::ExtractArchive { .. }), "{error}"); + assert!(!temp.path().join("clickhouse").exists()); + assert!(!temp.path().join(".clickhouse.extracting").exists()); + } + #[test] fn test_parse_version_output_client() { let output = "ClickHouse client version 25.12.9.61 (official build)."; @@ -506,20 +490,46 @@ mod tests { } #[test] - fn malformed_archive_error_includes_archive_path_and_cause() { + fn corrupted_gzip_crc_is_not_committed() { + assert_invalid_gzip_is_not_committed(|archive_path| { + let mut bytes = fs::read(archive_path).unwrap(); + let crc_offset = bytes.len() - 8; + bytes[crc_offset] ^= 0xff; + fs::write(archive_path, bytes).unwrap(); + }); + } + + #[test] + fn truncated_gzip_trailer_is_not_committed() { + assert_invalid_gzip_is_not_committed(|archive_path| { + let file = fs::OpenOptions::new() + .write(true) + .open(archive_path) + .unwrap(); + file.set_len(file.metadata().unwrap().len() - 4).unwrap(); + }); + } + + #[test] + fn malformed_archive_error_preserves_archive_destination_and_cause() { let temp = tempfile::tempdir().unwrap(); let archive_path = temp.path().join("broken.tgz"); fs::write(&archive_path, "not a gzip archive").unwrap(); + let destination = temp.path().join("clickhouse"); let error = extract_tarball_auto(&archive_path, temp.path()).unwrap_err(); - let message = error.to_string(); - - assert!( - message.contains(&archive_path.display().to_string()), - "{message}" - ); - assert!(message.contains("invalid gzip header"), "{message}"); - assert_ne!(message, "Extraction failed: tar extraction failed"); + let Error::ExtractArchive { + archive, + destination: error_destination, + source, + } = &error + else { + panic!("expected contextual archive error: {error}"); + }; + + assert_eq!(archive, &archive_path); + assert_eq!(error_destination, &destination); + assert!(error.to_string().contains(&source.to_string())); assert!(!temp.path().join("clickhouse").exists()); }