From 0aad4e2246971601c16a20240eebf61964f8c198 Mon Sep 17 00:00:00 2001 From: Florian Klink Date: Tue, 17 Jan 2023 11:56:23 +0100 Subject: feat(tvix/derivation): also fail if output name is called `drv` `drv` is an invalid output name too, as this would cause a `builtins.derivation` call to return an attrset with a `drvPath` key (which already exists) and has a different meaning. Also handle errors during store path construction, and return our own error type, instead of the ParseStorePathError. Change-Id: Ib7952dde1d5cf18a0e210928df7c57b5939b7678 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7850 Tested-by: BuildkiteCI Reviewed-by: tazjin Autosubmit: flokli --- tvix/derivation/src/derivation.rs | 16 +++++++++++----- tvix/derivation/src/output.rs | 2 -- tvix/derivation/src/validate.rs | 24 +++++++++++++++++++++--- 3 files changed, 32 insertions(+), 10 deletions(-) diff --git a/tvix/derivation/src/derivation.rs b/tvix/derivation/src/derivation.rs index 1e2605f5b931..daccfad6c42e 100644 --- a/tvix/derivation/src/derivation.rs +++ b/tvix/derivation/src/derivation.rs @@ -1,12 +1,12 @@ -use crate::nix_hash; use crate::output::{Hash, Output}; use crate::write; +use crate::{nix_hash, ValidateDerivationError}; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use std::collections::BTreeSet; use std::{collections::BTreeMap, fmt, fmt::Write}; use tvix_store::nixbase32::NIXBASE32; -use tvix_store::store_path::{ParseStorePathError, StorePath, STORE_DIR}; +use tvix_store::store_path::{StorePath, STORE_DIR}; #[derive(Clone, Debug, Default, Eq, PartialEq, Serialize, Deserialize)] pub struct Derivation { @@ -35,7 +35,7 @@ fn build_store_path( is_derivation: bool, path_hash: &[u8], name: &str, -) -> Result { +) -> Result { let compressed = nix_hash::compress_hash(path_hash, 20); if is_derivation { StorePath::from_string( @@ -50,6 +50,9 @@ fn build_store_path( } else { StorePath::from_string(format!("{}-{}", NIXBASE32.encode(&compressed), name,).as_str()) } + .map_err(|_e| ValidateDerivationError::InvalidOutputName(name.to_string())) + // Constructing the StorePath can only fail if the passed output name was + // invalid, so map errors to a [ValidateDerivationError::InvalidOutputName]. } impl Derivation { @@ -107,7 +110,10 @@ impl Derivation { /// - Take the digest, run hash.CompressHash(digest, 20) on it. /// - Encode it with nixbase32 /// - Use it (and the name) to construct a [StorePath]. - pub fn calculate_derivation_path(&self, name: &str) -> Result { + pub fn calculate_derivation_path( + &self, + name: &str, + ) -> Result { let mut hasher = Sha256::new(); // collect the list of paths from input_sources and input_derivations @@ -224,7 +230,7 @@ impl Derivation { &mut self, name: &str, drv_replacement_str: &str, - ) -> Result<(), ParseStorePathError> { + ) -> Result<(), ValidateDerivationError> { let mut hasher = Sha256::new(); // Check if the Derivation is fixed output, because they cause diff --git a/tvix/derivation/src/output.rs b/tvix/derivation/src/output.rs index cd77c00f2f41..b8bd49fbbb18 100644 --- a/tvix/derivation/src/output.rs +++ b/tvix/derivation/src/output.rs @@ -1,8 +1,6 @@ use serde::{Deserialize, Serialize}; use tvix_store::store_path::{ParseStorePathError, StorePath}; -use crate::ValidateDerivationError; - #[derive(Clone, Debug, Default, Eq, PartialEq, Serialize, Deserialize)] pub struct Output { pub path: String, diff --git a/tvix/derivation/src/validate.rs b/tvix/derivation/src/validate.rs index 2ff23d52048f..b5ba4f72ca28 100644 --- a/tvix/derivation/src/validate.rs +++ b/tvix/derivation/src/validate.rs @@ -12,7 +12,16 @@ impl Derivation { // Validate all outputs for (output_name, output) in &self.outputs { - if output_name.is_empty() { + // empty output names are invalid. + // + // `drv` is an invalid output name too, as this would cause + // a `builtins.derivation` call to return an attrset with a + // `drvPath` key (which already exists) and has a different + // meaning. + // + // Other output names that don't match the name restrictions from + // [StorePath] will fail output path calculation. + if output_name.is_empty() || output_name == "drv" { return Err(ValidateDerivationError::InvalidOutputName( output_name.to_string(), )); @@ -62,13 +71,22 @@ impl Derivation { } for output_name in output_names.iter() { - if output_name.is_empty() { + // empty output names are invalid. + // + // `drv` is an invalid output name too, as this would cause + // a `builtins.derivation` call to return an attrset with a + // `drvPath` key (which already exists) and has a different + // meaning. + // + // Other output names that don't match the name restrictions + // from [StorePath] can't be constructed with this library, but + // are not explicitly checked here (yet). + if output_name.is_empty() || output_name == "drv" { return Err(ValidateDerivationError::InvalidInputDerivationOutputName( input_derivation_path.to_string(), output_name.to_string(), )); } - // TODO: do we need to apply more name validation here? } } -- cgit 1.4.1