feat(tvix/derivation): also fail if output name is called `drv` r/5694

`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 <tazjin@tvl.su>
Autosubmit: flokli <flokli@flokli.de>

3 files changed, 32 insertions, 10 deletions

diff --git a/tvix/derivation/src/derivation.rs b/tvix/derivation/src/derivation.rs
index 1e2605f5b9..daccfad6c4 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<StorePath, ParseStorePathError> {
+) -> Result<StorePath, ValidateDerivationError> {
     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<StorePath, ParseStorePathError> {
+    pub fn calculate_derivation_path(
+        &self,
+        name: &str,
+    ) -> Result<StorePath, ValidateDerivationError> {
         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 cd77c00f2f..b8bd49fbbb 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 2ff23d5204..b5ba4f72ca 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?
             }
         }