From e771cb81a5b1649489ef6e26bb244d8947da1fd8 Mon Sep 17 00:00:00 2001 From: "Sean P. Kelly" Date: Wed, 18 Dec 2024 21:48:57 +0000 Subject: [PATCH] twoliter: ensure krane tmpfile is deleted When using named tempfiles or tempdirs, the `tempfile` crate relies on Rust calling `drop()` in order to delete the created temporary files. We were using a static reference to a tempfile of the krane binary, which is not guaranteed to drop when the program completes. This caused twoliter to leak somewhat large krane binary tempfiles. This commit moves away from a static reference to the tempfile, instead improving ergonomics around ImageTool owning the tempfile reference. This allows the Rust compiler to more-reliably drop() the reference on exit. --- Cargo.lock | 13 +++++ Cargo.toml | 1 + tools/krane/Cargo.toml | 1 + tools/krane/src/lib.rs | 23 ++++++-- tools/oci-cli-wrapper/src/crane.rs | 24 +++++++- tools/oci-cli-wrapper/src/lib.rs | 75 ++++++++++++++++++++----- tools/pubsys/src/kit/publish_kit/mod.rs | 7 ++- twoliter/Cargo.toml | 1 + twoliter/src/project/lock/mod.rs | 13 ++--- twoliter/src/project/mod.rs | 19 ++++++- twoliter/src/tools.rs | 6 +- 11 files changed, 149 insertions(+), 34 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5231fc94..64af0e58 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1145,6 +1145,17 @@ dependencies = [ "powerfmt", ] +[[package]] +name = "derivative" +version = "2.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fcc3dd5e9e9c0b295d6e1e4d811fb6f157d5ffd784b8d202fc62eac8035a770b" +dependencies = [ + "proc-macro2", + "quote", + "syn 1.0.109", +] + [[package]] name = "digest" version = "0.10.7" @@ -1904,6 +1915,7 @@ dependencies = [ "lazy_static", "tar", "tempfile", + "tokio", "which", ] @@ -3868,6 +3880,7 @@ dependencies = [ "buildsys-config", "bytes", "clap", + "derivative", "env_logger", "filetime", "flate2", diff --git a/Cargo.toml b/Cargo.toml index 7e1f8b9f..fea67abd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -85,6 +85,7 @@ chrono = { version = "0.4", default-features = false } clap = "4" coldsnap = { version = "0.6", default-features = false } daemonize = "0.5" +derivative = "2" duct = "0.13" env_logger = "0.11" fastrand = "2" diff --git a/tools/krane/Cargo.toml b/tools/krane/Cargo.toml index 74202a41..b6cd4c96 100644 --- a/tools/krane/Cargo.toml +++ b/tools/krane/Cargo.toml @@ -11,6 +11,7 @@ anyhow.workspace = true flate2.workspace = true lazy_static.workspace = true tempfile.workspace = true +tokio.workspace = true [build-dependencies] flate2.workspace = true diff --git a/tools/krane/src/lib.rs b/tools/krane/src/lib.rs index 2ffd3503..33c3e45a 100644 --- a/tools/krane/src/lib.rs +++ b/tools/krane/src/lib.rs @@ -2,15 +2,13 @@ use anyhow::Result; use flate2::read::GzDecoder; use std::fs::{File, Permissions}; use std::os::unix::fs::PermissionsExt; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use tempfile::TempDir; const COMPRESSED_KRANE_BIN: &[u8] = include_bytes!(env!("KRANE_GZ_PATH")); -lazy_static::lazy_static! { - pub static ref KRANE: Krane = Krane::seal().unwrap(); -} +pub type KraneError = anyhow::Error; #[derive(Debug)] pub struct Krane { @@ -20,9 +18,11 @@ pub struct Krane { } impl Krane { - fn seal() -> Result { + pub fn new() -> Result { let tmp_dir = TempDir::new()?; + let path = tmp_dir.path().join("krane"); + Self::write_archive(&path)?; let mut krane_file = File::create(&path)?; let permissions = Permissions::from_mode(0o755); @@ -38,6 +38,17 @@ impl Krane { }) } + pub fn write_archive(path: impl AsRef) -> Result<()> { + let mut krane_file = File::create(path.as_ref())?; + let permissions = Permissions::from_mode(0o755); + krane_file.set_permissions(permissions)?; + + let mut krane_reader = GzDecoder::new(COMPRESSED_KRANE_BIN); + + std::io::copy(&mut krane_reader, &mut krane_file)?; + Ok(()) + } + pub fn path(&self) -> &PathBuf { &self.path } @@ -50,7 +61,7 @@ mod test { #[test] fn test_krane_runs() { - let status = Command::new(KRANE.path()) + let status = Command::new(Krane::new().unwrap().path()) .arg("--help") .output() .expect("failed to run krane"); diff --git a/tools/oci-cli-wrapper/src/crane.rs b/tools/oci-cli-wrapper/src/crane.rs index 1841ac38..15cee659 100644 --- a/tools/oci-cli-wrapper/src/crane.rs +++ b/tools/oci-cli-wrapper/src/crane.rs @@ -1,7 +1,10 @@ +use std::borrow::Borrow; +use std::fmt::Debug; use std::fs::File; use std::path::Path; use async_trait::async_trait; +use krane_bundle::Krane; use snafu::ResultExt; use tar::Archive as TarArchive; use tempfile::TempDir; @@ -10,12 +13,27 @@ use crate::{ cli::CommandLine, error, ConfigView, DockerArchitecture, ImageToolImpl, ImageView, Result, }; +// allow CraneCLI to be initialized with an Arc or an owned Krane +pub trait CraneBinary: Borrow + Debug + Send + Sync + 'static {} +impl + Debug + Send + Sync + 'static> CraneBinary for T {} + #[derive(Debug)] -pub struct CraneCLI { +pub struct CraneCLI { pub(crate) cli: CommandLine, + // Hold a reference to a tempfile which is deleted when `_krane` is dropped + _crane: C, } -impl CraneCLI { +impl CraneCLI { + pub fn new(crane: C) -> Self { + Self { + cli: CommandLine { + path: crane.borrow().path().to_path_buf(), + }, + _crane: crane, + } + } + /// Enables verbose logging of crane if debug logging is enabled. fn crane_cmd<'a>(cmd: &[&'a str]) -> Vec<&'a str> { if log::max_level() >= log::LevelFilter::Debug { @@ -27,7 +45,7 @@ impl CraneCLI { } #[async_trait] -impl ImageToolImpl for CraneCLI { +impl ImageToolImpl for CraneCLI { async fn pull_oci_image(&self, path: &Path, uri: &str) -> Result<()> { let archive_path = path.to_string_lossy(); self.cli diff --git a/tools/oci-cli-wrapper/src/lib.rs b/tools/oci-cli-wrapper/src/lib.rs index 1949543f..4af4594a 100644 --- a/tools/oci-cli-wrapper/src/lib.rs +++ b/tools/oci-cli-wrapper/src/lib.rs @@ -12,12 +12,13 @@ //! metadata. In addition, in order to operate with OCI image format, the containerd-snapshotter //! feature has to be enabled in the docker daemon use std::fmt::{Display, Formatter}; +use std::sync::Arc; use std::{collections::HashMap, path::Path}; use async_trait::async_trait; -use cli::CommandLine; -use crane::CraneCLI; -use krane_bundle::KRANE; +use crane::{CraneBinary, CraneCLI}; +use error::CraneInitializeSnafu; +use krane_bundle::Krane; use olpc_cjson::CanonicalFormatter; use serde::{Deserialize, Serialize}; use snafu::ResultExt; @@ -25,23 +26,30 @@ use snafu::ResultExt; mod cli; mod crane; -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct ImageTool { - image_tool_impl: Box, + image_tool_impl: Arc, } impl ImageTool { - /// Uses the builtin `krane` provided by the `tools/krane` crate. - pub fn from_builtin_krane() -> Self { - let image_tool_impl = Box::new(CraneCLI { - cli: CommandLine { - path: KRANE.path().to_path_buf(), - }, - }); + /// Creates a new `ImageTool` using the given Krane. + /// + /// Can be called with an owned Krane or Arc. + pub fn from_krane(krane: K) -> Self { + let image_tool_impl = Arc::new(CraneCLI::new(krane)); Self { image_tool_impl } } - pub fn new(image_tool_impl: Box) -> Self { + /// Uses the builtin `krane` provided by the `tools/krane` crate. + /// + /// Each call to this creates a new temporary file with the `krane` binary. Use + /// [`ImageTool::from_krane`] to share references to e.g. an `Arc`. + pub fn from_new_krane() -> Result { + let image_tool_impl = Arc::new(CraneCLI::new(Krane::new().context(CraneInitializeSnafu)?)); + Ok(Self { image_tool_impl }) + } + + pub fn new(image_tool_impl: Arc) -> Self { Self { image_tool_impl } } @@ -179,6 +187,9 @@ pub mod error { #[snafu(display("Failed to create temporary directory for crane push: {source}"))] CraneTemp { source: std::io::Error }, + #[snafu(display("Failed to initialize crane: {source}"))] + CraneInitialize { source: krane_bundle::KraneError }, + #[snafu(display("Failed to create temporary directory for docker save: {source}"))] DockerTemp { source: std::io::Error }, @@ -220,3 +231,41 @@ pub mod error { Unsupported { name: String }, } } + +#[cfg(test)] +mod test { + use std::sync::Arc; + + use super::*; + + #[tokio::test] + async fn test_owned_krane_cleans_up() { + // Ensure we can make an image tool with an owned or Arc Krane object. + // Using an Arc would allow multiple `ImageTools` to refer to the same `krane` binary on + // disk. + + let krane = Krane::new().unwrap(); + let bin_path = krane.path().to_path_buf(); + + let owned_image_tool = ImageTool::from_krane(krane); + assert!(tokio::fs::try_exists(&bin_path).await.unwrap()); + + drop(owned_image_tool); + assert!(!tokio::fs::try_exists(&bin_path).await.unwrap()); + } + + #[tokio::test] + async fn test_shared_krane_cleans_up() { + let krane = Arc::new(Krane::new().unwrap()); + let bin_path = krane.path().to_path_buf(); + + let image_tool = ImageTool::from_krane(Arc::clone(&krane)); + assert!(tokio::fs::try_exists(&bin_path).await.unwrap()); + + drop(image_tool); + assert!(tokio::fs::try_exists(&bin_path).await.unwrap()); + + drop(krane); + assert!(!tokio::fs::try_exists(&bin_path).await.unwrap()); + } +} diff --git a/tools/pubsys/src/kit/publish_kit/mod.rs b/tools/pubsys/src/kit/publish_kit/mod.rs index 07aff00a..ca046ffe 100644 --- a/tools/pubsys/src/kit/publish_kit/mod.rs +++ b/tools/pubsys/src/kit/publish_kit/mod.rs @@ -31,7 +31,7 @@ pub(crate) struct PublishKitArgs { } pub(crate) async fn run(args: &Args, publish_kit_args: &PublishKitArgs) -> Result<()> { - let image_tool = ImageTool::from_builtin_krane(); + let image_tool = ImageTool::from_new_krane().context(error::ImageToolInitializationSnafu)?; // If a lock file exists, use that, otherwise use Infra.toml let infra_config = InfraConfig::from_path_or_lock(&args.infra_config_path, false) @@ -137,6 +137,11 @@ mod error { #[snafu(display("Error reading config: {}", source))] Config { source: pubsys_config::Error }, + #[snafu(display("Failed to initialize oci image tool: {source}"))] + ImageToolInitialization { + source: oci_cli_wrapper::error::Error, + }, + #[snafu(display("Could not convert {} to docker architecture: {}", arch, source))] InvalidArchitecture { source: oci_cli_wrapper::error::Error, diff --git a/twoliter/Cargo.toml b/twoliter/Cargo.toml index 1aaf6c05..af3fd2c1 100644 --- a/twoliter/Cargo.toml +++ b/twoliter/Cargo.toml @@ -17,6 +17,7 @@ async-trait.workspace = true base64.workspace = true buildsys-config.workspace = true clap = { workspace = true, features = ["derive", "env", "std"] } +derivative.workspace = true env_logger.workspace = true filetime.workspace = true flate2.workspace = true diff --git a/twoliter/src/project/lock/mod.rs b/twoliter/src/project/lock/mod.rs index 6d9a07b9..5e5110a3 100644 --- a/twoliter/src/project/lock/mod.rs +++ b/twoliter/src/project/lock/mod.rs @@ -19,7 +19,6 @@ use crate::project::{Project, ValidIdentifier}; use crate::schema_version::SchemaVersion; use anyhow::{bail, ensure, Context, Result}; use image::{ImageResolver, LockedImage}; -use oci_cli_wrapper::ImageTool; use olpc_cjson::CanonicalFormatter as CanonicalJsonFormatter; use semver::Version; use serde::{Deserialize, Serialize}; @@ -103,10 +102,9 @@ impl LockedSDK { }; debug!(?sdk, "Resolving workspace SDK"); - let image_tool = ImageTool::from_builtin_krane(); ImageResolver::from_image(&sdk)? .skip_metadata_retrieval() // SDKs don't have metadata - .resolve(&image_tool) + .resolve(project.oci_image_tool()) .await .map(|(sdk, _)| Some(Self(sdk))) } @@ -203,7 +201,6 @@ impl Lock { /// Fetches all external kits defined in a Twoliter.lock to the build directory #[instrument(level = "trace", skip_all)] pub(crate) async fn fetch(&self, project: &Project, arch: &str) -> Result<()> { - let image_tool = ImageTool::from_builtin_krane(); let target_dir = project.external_kits_dir(); create_dir_all(&target_dir).await.context(format!( "failed to create external-kits directory at {}", @@ -218,7 +215,7 @@ impl Lock { let image = project.as_project_image(image)?; let resolver = ImageResolver::from_image(&image)?; resolver - .extract(&image_tool, &project.external_kits_dir(), arch) + .extract(project.oci_image_tool(), &project.external_kits_dir(), arch) .await?; } @@ -257,7 +254,6 @@ impl Lock { async fn resolve(project: &Project) -> Result { let mut known: HashMap<(ValidIdentifier, ValidIdentifier), Version> = HashMap::new(); let mut locked: Vec = Vec::new(); - let image_tool = ImageTool::from_builtin_krane(); let mut remaining = project.direct_kit_deps()?; let mut sdk_set = HashSet::new(); @@ -292,7 +288,8 @@ impl Lock { image.version().clone(), ); let image_resolver = ImageResolver::from_image(image)?; - let (locked_image, metadata) = image_resolver.resolve(&image_tool).await?; + let (locked_image, metadata) = + image_resolver.resolve(project.oci_image_tool()).await?; let metadata = metadata.context(format!( "failed to validate kit image with name {} from vendor {}", locked_image.name, locked_image.vendor @@ -322,7 +319,7 @@ impl Lock { debug!(?sdk, "Resolving workspace SDK"); let (sdk, _metadata) = ImageResolver::from_image(sdk)? .skip_metadata_retrieval() // SDKs don't have metadata - .resolve(&image_tool) + .resolve(project.oci_image_tool()) .await?; Ok(Self { diff --git a/twoliter/src/project/mod.rs b/twoliter/src/project/mod.rs index 8fe87f5f..3decc546 100644 --- a/twoliter/src/project/mod.rs +++ b/twoliter/src/project/mod.rs @@ -2,7 +2,9 @@ mod lock; pub(crate) mod vendor; pub(crate) use self::vendor::ArtifactVendor; +use krane_bundle::Krane; pub(crate) use lock::VerificationTagger; +use oci_cli_wrapper::ImageTool; use self::lock::{Lock, LockedSDK, Override}; use crate::common::fs::{self, read_to_string}; @@ -14,6 +16,7 @@ use async_recursion::async_recursion; use async_trait::async_trait; use async_walkdir::WalkDir; use buildsys_config::{EXTERNAL_KIT_DIRECTORY, EXTERNAL_KIT_METADATA}; +use derivative::Derivative; use futures::stream::StreamExt; use semver::Version; use serde::de::Error; @@ -46,7 +49,8 @@ pub(crate) async fn load_or_find_project(user_path: Option) -> Result

{ filepath: PathBuf, project_dir: PathBuf, @@ -70,6 +74,11 @@ pub(crate) struct Project { /// The resolved and locked dependencies of the project. lock: L, + + #[derivative(PartialEq = "ignore")] + #[derivative(Ord = "ignore")] + #[derivative(PartialOrd = "ignore")] + oci_image_tool: ImageTool, } impl Project { @@ -155,6 +164,7 @@ impl Project { kit: self.kit.clone(), overrides: self.overrides.clone(), lock: new_lock.into(), + oci_image_tool: self.oci_image_tool.clone(), } } @@ -272,6 +282,10 @@ impl Project { modules.sort(); Ok(modules) } + + pub(crate) fn oci_image_tool(&self) -> &ImageTool { + &self.oci_image_tool + } } impl Project { @@ -531,6 +545,9 @@ impl UnvalidatedProject { kit: self.kit.unwrap_or_default(), overrides, lock: Unlocked, + oci_image_tool: ImageTool::from_krane( + Krane::new().context("Failed to initialize `krane`")?, + ), }) } diff --git a/twoliter/src/tools.rs b/twoliter/src/tools.rs index fe5b1c32..ab10ad6c 100644 --- a/twoliter/src/tools.rs +++ b/twoliter/src/tools.rs @@ -2,7 +2,7 @@ use crate::common::fs; use anyhow::{Context, Result}; use filetime::{set_file_handle_times, set_file_mtime, FileTime}; use flate2::read::ZlibDecoder; -use krane_bundle::KRANE; +use krane_bundle::Krane; use std::path::Path; use tar::Archive; use tokio::fs::OpenOptions; @@ -50,7 +50,9 @@ pub(crate) async fn install_tools(tools_dir: impl AsRef) -> Result<()> { write_bin("testsys", TESTSYS, &dir, mtime).await?; write_bin("tuftool", TUFTOOL, &dir, mtime).await?; write_bin("unplug", UNPLUG, &dir, mtime).await?; - fs::copy(KRANE.path(), dir.join("krane")).await?; + + let krane_path = dir.join("krane"); + tokio::task::spawn_blocking(|| Krane::write_archive(krane_path)).await??; // Apply the mtime to the directory now that the writes are done. set_file_mtime(dir, mtime).context(format!("Unable to set mtime for '{}'", dir.display()))?;