fix(nix-compat/derivation): handle dups r/6832

This now properly checks for duplicate output names in input derivation
output names, and duplicate input sources.

Change-Id: I0053854bfbf504f4f511fb3fe1a77a82b3aa61dd
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9738
Autosubmit: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>

2 files changed, 173 insertions, 23 deletions

diff --git a/tvix/nix-compat/src/derivation/parse_error.rs b/tvix/nix-compat/src/derivation/parse_error.rs
index a064d4faba7b..34a9c9fd9204 100644
--- a/tvix/nix-compat/src/derivation/parse_error.rs
+++ b/tvix/nix-compat/src/derivation/parse_error.rs
@@ -6,15 +6,23 @@ use crate::nixhash;
 
 pub type NomResult<I, O> = IResult<I, O, NomError<I>>;
 
-#[derive(Debug, PartialEq)]
+#[derive(Debug, thiserror::Error, PartialEq)]
 pub enum ErrorKind {
-    // duplicate key in map
+    /// duplicate key in map
+    #[error("duplicate map key: {0}")]
     DuplicateMapKey(String),
 
-    // Digest parsing error
+    /// Input derivation has two outputs with the same name
+    #[error("duplicate output name {1} for input derivation {0}")]
+    DuplicateInputDerivationOutputName(String, String),
+
+    #[error("duplicate input source: {0}")]
+    DuplicateInputSource(String),
+
+    #[error("nix hash error: {0}")]
     NixHashError(nixhash::Error),
 
-    // error kind wrapped from native nom errors
+    #[error("nom error: {0:?}")]
     Nom(nom::error::ErrorKind),
 }
 
diff --git a/tvix/nix-compat/src/derivation/parser.rs b/tvix/nix-compat/src/derivation/parser.rs
index dfcd327e4f0f..2b5a8041592b 100644
--- a/tvix/nix-compat/src/derivation/parser.rs
+++ b/tvix/nix-compat/src/derivation/parser.rs
@@ -125,8 +125,45 @@ fn parse_outputs(i: &[u8]) -> NomResult<&[u8], BTreeMap<String, Output>> {
     }
 }
 
-fn parse_input_derivations(i: &[u8]) -> NomResult<&[u8], BTreeMap<String, Vec<String>>> {
-    parse_kv::<Vec<String>, _>(aterm::parse_str_list)(i)
+fn parse_input_derivations(i: &[u8]) -> NomResult<&[u8], BTreeMap<String, BTreeSet<String>>> {
+    let (i, input_derivations_list) = parse_kv::<Vec<String>, _>(aterm::parse_str_list)(i)?;
+
+    // This is a HashMap of drv paths to a list of output names.
+    let mut input_derivations: BTreeMap<String, BTreeSet<String>> = BTreeMap::new();
+
+    for (input_derivation, output_names) in input_derivations_list {
+        let mut new_output_names = BTreeSet::new();
+        for output_name in output_names.into_iter() {
+            if !new_output_names.insert(output_name.clone()) {
+                return Err(nom::Err::Failure(NomError {
+                    input: i,
+                    code: ErrorKind::DuplicateInputDerivationOutputName(
+                        input_derivation.to_string(),
+                        output_name.to_string(),
+                    ),
+                }));
+            }
+        }
+        input_derivations.insert(input_derivation, new_output_names);
+    }
+
+    Ok((i, input_derivations))
+}
+
+fn parse_input_sources(i: &[u8]) -> NomResult<&[u8], BTreeSet<String>> {
+    let (i, input_sources_lst) = aterm::parse_str_list(i).map_err(into_nomerror)?;
+
+    let mut input_sources: BTreeSet<_> = BTreeSet::new();
+    for input_source in input_sources_lst.into_iter() {
+        if !input_sources.insert(input_source.clone()) {
+            return Err(nom::Err::Failure(NomError {
+                input: i,
+                code: ErrorKind::DuplicateInputSource(input_source),
+            }));
+        }
+    }
+
+    Ok((i, input_sources))
 }
 
 pub fn parse_derivation(i: &[u8]) -> NomResult<&[u8], Derivation> {
@@ -144,7 +181,7 @@ pub fn parse_derivation(i: &[u8]) -> NomResult<&[u8], Derivation> {
                 // // parse input derivations
                 terminated(parse_input_derivations, nomchar(',')),
                 // // parse input sources
-                |i| terminated(aterm::parse_str_list, nomchar(','))(i).map_err(into_nomerror),
+                terminated(parse_input_sources, nomchar(',')),
                 // // parse system
                 |i| terminated(aterm::parse_string_field, nomchar(','))(i).map_err(into_nomerror),
                 // // parse builder
@@ -166,24 +203,12 @@ pub fn parse_derivation(i: &[u8]) -> NomResult<&[u8], Derivation> {
                 arguments,
                 environment,
             )| {
-                // All values in input_derivations need to be converted from
-                // Vec<String> to BTreeSet<String>
-                let mut input_derivations_new: BTreeMap<_, BTreeSet<_>> = BTreeMap::new();
-                for (k, v) in input_derivations.into_iter() {
-                    let values_new: BTreeSet<_> = BTreeSet::from_iter(v.into_iter());
-                    input_derivations_new.insert(k, values_new);
-                    // TODO: actually check they're not duplicate in the parser side!
-                }
-
-                // Input sources need to be converted from Vec<_> to BTreeSet<_>
-                let input_sources_new: BTreeSet<_> = BTreeSet::from_iter(input_sources);
-
                 Derivation {
                     arguments,
                     builder,
                     environment,
-                    input_derivations: input_derivations_new,
-                    input_sources: input_sources_new,
+                    input_derivations,
+                    input_sources,
                     outputs,
                     system,
                 }
@@ -246,9 +271,9 @@ where
 
 #[cfg(test)]
 mod tests {
-    use std::collections::BTreeMap;
+    use std::collections::{BTreeMap, BTreeSet};
 
-    use crate::derivation::Output;
+    use crate::derivation::{parse_error::ErrorKind, Output};
     use bstr::{BString, ByteSlice};
     use lazy_static::lazy_static;
     use test_case::test_case;
@@ -279,8 +304,44 @@ mod tests {
             b.insert("b".to_string(), b"2".as_bstr().to_owned());
             b
         };
+        static ref EXP_INPUT_DERIVATIONS_SIMPLE: BTreeMap<String, BTreeSet<String>> = {
+            let mut b = BTreeMap::new();
+            b.insert(
+                "/nix/store/8bjm87p310sb7r2r0sg4xrynlvg86j8k-hello-2.12.1.tar.gz.drv".to_string(),
+                {
+                    let mut output_names = BTreeSet::new();
+                    output_names.insert("out".to_string());
+                    output_names
+                },
+            );
+            b.insert(
+                "/nix/store/p3jc8aw45dza6h52v81j7lk69khckmcj-bash-5.2-p15.drv".to_string(),
+                {
+                    let mut output_names = BTreeSet::new();
+                    output_names.insert("out".to_string());
+                    output_names.insert("lib".to_string());
+                    output_names
+                },
+            );
+            b
+        };
+        static ref EXP_INPUT_DERIVATIONS_SIMPLE_ATERM: String = {
+            format!(
+                "[(\"{0}\",[\"out\"]),(\"{1}\",[\"out\",\"lib\"])]",
+                "/nix/store/8bjm87p310sb7r2r0sg4xrynlvg86j8k-hello-2.12.1.tar.gz.drv",
+                "/nix/store/p3jc8aw45dza6h52v81j7lk69khckmcj-bash-5.2-p15.drv"
+            )
+        };
+        static ref EXP_INPUT_SOURCES_SIMPLE: BTreeSet<String> = {
+            let mut b = BTreeSet::new();
+            b.insert("/nix/store/55lwldka5nyxa08wnvlizyqw02ihy8ic-has-multi-out".to_string());
+            b.insert("/nix/store/2vixb94v0hy2xc6p7mbnxxcyc095yyia-has-multi-out-lib".to_string());
+            b
+        };
     }
 
+    /// Ensure parsing KVs works
+    #[test_case(b"[]", &BTreeMap::new(), b""; "empty")]
     #[test_case(b"[(\"a\",\"1\"),(\"b\",\"2\")]", &EXP_AB_MAP, b""; "simple")]
     fn parse_kv(input: &'static [u8], expected: &BTreeMap<String, BString>, exp_rest: &[u8]) {
         let (rest, parsed) = super::parse_kv::<BString, _>(crate::aterm::parse_bstr_field)(input)
@@ -289,6 +350,87 @@ mod tests {
         assert_eq!(*expected, parsed);
     }
 
+    /// Ensures the kv parser complains about duplicate map keys
+    #[test]
+    fn parse_kv_fail_dup_keys() {
+        let input: &'static [u8] = b"[(\"a\",\"1\"),(\"a\",\"2\")]";
+        let e = super::parse_kv::<BString, _>(crate::aterm::parse_bstr_field)(input)
+            .expect_err("must fail");
+
+        match e {
+            nom::Err::Failure(e) => {
+                assert_eq!(ErrorKind::DuplicateMapKey("a".to_string()), e.code);
+            }
+            _ => panic!("unexpected error"),
+        }
+    }
+
+    /// Ensure parsing input derivations works.
+    #[test_case(b"[]", &BTreeMap::new(); "empty")]
+    #[test_case(EXP_INPUT_DERIVATIONS_SIMPLE_ATERM.as_bytes(), &EXP_INPUT_DERIVATIONS_SIMPLE; "simple")]
+    fn parse_input_derivations(
+        input: &'static [u8],
+        expected: &BTreeMap<String, BTreeSet<String>>,
+    ) {
+        let (rest, parsed) = super::parse_input_derivations(input).expect("must parse");
+
+        assert_eq!(expected, &parsed, "parsed mismatch");
+        assert!(rest.is_empty(), "rest must be empty");
+    }
+
+    /// Ensures the input derivation parser complains about duplicate output names
+    #[test]
+    fn parse_input_derivations_fail_dup_output_names() {
+        let input_str = format!(
+            "[(\"{0}\",[\"out\"]),(\"{1}\",[\"out\",\"out\"])]",
+            "/nix/store/8bjm87p310sb7r2r0sg4xrynlvg86j8k-hello-2.12.1.tar.gz.drv",
+            "/nix/store/p3jc8aw45dza6h52v81j7lk69khckmcj-bash-5.2-p15.drv"
+        );
+        let e = super::parse_input_derivations(input_str.as_bytes()).expect_err("must fail");
+
+        match e {
+            nom::Err::Failure(e) => {
+                assert_eq!(
+                    ErrorKind::DuplicateInputDerivationOutputName(
+                        "/nix/store/p3jc8aw45dza6h52v81j7lk69khckmcj-bash-5.2-p15.drv".to_string(),
+                        "out".to_string()
+                    ),
+                    e.code
+                );
+            }
+            _ => panic!("unexpected error"),
+        }
+    }
+
+    /// Ensure parsing input sources works
+    #[test_case(b"[]", &BTreeSet::new(); "empty")]
+    #[test_case(b"[\"/nix/store/55lwldka5nyxa08wnvlizyqw02ihy8ic-has-multi-out\",\"/nix/store/2vixb94v0hy2xc6p7mbnxxcyc095yyia-has-multi-out-lib\"]", &EXP_INPUT_SOURCES_SIMPLE; "simple")]
+    fn parse_input_sources(input: &'static [u8], expected: &BTreeSet<String>) {
+        let (rest, parsed) = super::parse_input_sources(input).expect("must parse");
+
+        assert_eq!(expected, &parsed, "parsed mismatch");
+        assert!(rest.is_empty(), "rest must be empty");
+    }
+
+    /// Ensures the input sources parser complains about duplicate input sources
+    #[test]
+    fn parse_input_sources_fail_dup_keys() {
+        let input: &'static [u8] = b"[\"/nix/store/55lwldka5nyxa08wnvlizyqw02ihy8ic-foo\",\"/nix/store/55lwldka5nyxa08wnvlizyqw02ihy8ic-foo\"]";
+        let e = super::parse_input_sources(input).expect_err("must fail");
+
+        match e {
+            nom::Err::Failure(e) => {
+                assert_eq!(
+                    ErrorKind::DuplicateInputSource(
+                        "/nix/store/55lwldka5nyxa08wnvlizyqw02ihy8ic-foo".to_string()
+                    ),
+                    e.code
+                );
+            }
+            _ => panic!("unexpected error"),
+        }
+    }
+
     #[test_case(
         br#"("out","/nix/store/5vyvcwah9l9kf07d52rcgdk70g2f4y13-foo","","")"#,
         ("out".to_string(), Output {