feat(tvix/nix-compat): have StorePath accept bytes r/6433

The primary constructor for this is now from_bytes, from_string is
simply calling .as_bytes() on the string, passing it along.

The InvalidName error now contains a Vec<u8>, to encode the invalid name
(which might not be a string anymore).

from_absolute_path now accepts a &[u8] (even though we might want to
make this a OSString of some sort).

StorePath::validate_name has been degraded to a pub(crate) function.
It's still used in src/derivation, even though it probably shouldn't at
all - that cleanup is left for cl/8412 though.

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

2 files changed, 66 insertions, 54 deletions

diff --git a/tvix/nix-compat/src/store_path/mod.rs b/tvix/nix-compat/src/store_path/mod.rs
index f7d42b2f7ef4..74dc5949076b 100644
--- a/tvix/nix-compat/src/store_path/mod.rs
+++ b/tvix/nix-compat/src/store_path/mod.rs
@@ -1,7 +1,10 @@
 use crate::nixbase32::{self, Nixbase32DecodeError};
-use std::{fmt, path::PathBuf};
+use std::{fmt, path::PathBuf, str::FromStr};
 use thiserror::Error;
 
+#[cfg(target_family = "unix")]
+use std::os::unix::ffi::OsStringExt;
+
 mod utils;
 
 pub use utils::*;
@@ -25,8 +28,8 @@ pub enum Error {
     InvalidHashEncoding(Nixbase32DecodeError),
     #[error("Invalid length")]
     InvalidLength(),
-    #[error("Invalid name: {0}")]
-    InvalidName(String),
+    #[error("Invalid name: {0:?}")]
+    InvalidName(Vec<u8>),
     #[error("Tried to parse an absolute path which was missing the store dir prefix.")]
     MissingStoreDir(),
 }
@@ -48,10 +51,20 @@ pub struct StorePath {
     pub name: String,
 }
 
+impl FromStr for StorePath {
+    type Err = Error;
+
+    /// Construct a [StorePath] by passing the `$digest-$name` string
+    /// that comes after [STORE_DIR_WITH_SLASH].
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        Self::from_bytes(s.as_bytes())
+    }
+}
+
 impl StorePath {
     /// Construct a [StorePath] by passing the `$digest-$name` string
     /// that comes after [STORE_DIR_WITH_SLASH].
-    pub fn from_string(s: &str) -> Result<StorePath, Error> {
+    pub fn from_bytes(s: &[u8]) -> Result<StorePath, Error> {
         // the whole string needs to be at least:
         //
         // - 32 characters (encoded hash)
@@ -61,19 +74,17 @@ impl StorePath {
             Err(Error::InvalidLength())?
         }
 
-        let digest = match nixbase32::decode(s[..ENCODED_DIGEST_SIZE].as_bytes()) {
+        let digest = match nixbase32::decode(&s[..ENCODED_DIGEST_SIZE]) {
             Ok(decoded) => decoded,
             Err(decoder_error) => return Err(Error::InvalidHashEncoding(decoder_error)),
         };
 
-        if s.as_bytes()[ENCODED_DIGEST_SIZE] != b'-' {
+        if s[ENCODED_DIGEST_SIZE] != b'-' {
             return Err(Error::MissingDash());
         }
 
-        StorePath::validate_name(&s[ENCODED_DIGEST_SIZE + 2..])?;
-
         Ok(StorePath {
-            name: s[ENCODED_DIGEST_SIZE + 1..].to_string(),
+            name: validate_name(&s[ENCODED_DIGEST_SIZE + 1..])?,
             digest: digest.try_into().expect("size is known"),
         })
     }
@@ -81,15 +92,16 @@ impl StorePath {
     /// Construct a [StorePath] from an absolute store path string.
     /// This is equivalent to calling [StorePath::from_string], but stripping
     /// the [STORE_DIR_WITH_SLASH] prefix before.
-    pub fn from_absolute_path(s: &str) -> Result<StorePath, Error> {
-        match s.strip_prefix(STORE_DIR_WITH_SLASH) {
-            Some(s_stripped) => Self::from_string(s_stripped),
+    pub fn from_absolute_path(s: &[u8]) -> Result<StorePath, Error> {
+        match s.strip_prefix(STORE_DIR_WITH_SLASH.as_bytes()) {
+            Some(s_stripped) => Self::from_bytes(s_stripped),
             None => Err(Error::MissingStoreDir()),
         }
     }
 
     /// Decompose a string into a [StorePath] and a [PathBuf] containing the
     /// rest of the path, or an error.
+    #[cfg(target_family = "unix")]
     pub fn from_absolute_path_full(s: &str) -> Result<(StorePath, PathBuf), Error> {
         // strip [STORE_DIR_WITH_SLASH] from s
         match s.strip_prefix(STORE_DIR_WITH_SLASH) {
@@ -102,17 +114,15 @@ impl StorePath {
                 let mut it = p.components();
 
                 // The first component of the rest must be parse-able as a [StorePath]
-                if let Some(s) = it.next() {
-                    // convert first component to string
-                    if let Some(s) = s.as_os_str().to_str() {
-                        let store_path = StorePath::from_string(s)?;
-                        let rest_buf: PathBuf = it.collect();
-                        Ok((store_path, rest_buf))
-                    } else {
-                        Err(Error::InvalidName("".to_string()))
-                    }
+                if let Some(first_component) = it.next() {
+                    // convert first component to StorePath
+                    let first_component_bytes = first_component.as_os_str().to_owned().into_vec();
+                    let store_path = StorePath::from_bytes(&first_component_bytes)?;
+                    // collect rest
+                    let rest_buf: PathBuf = it.collect();
+                    Ok((store_path, rest_buf))
                 } else {
-                    Err(Error::InvalidName("".to_string()))
+                    Err(Error::InvalidLength()) // Well, or missing "/"?
                 }
             }
         }
@@ -124,26 +134,27 @@ impl StorePath {
     pub fn to_absolute_path(&self) -> String {
         format!("{}{}", STORE_DIR_WITH_SLASH, self)
     }
+}
 
-    /// Checks a given &str to match the restrictions for store path names.
-    pub fn validate_name(s: &str) -> Result<(), Error> {
-        for c in s.chars() {
-            if c.is_ascii_alphanumeric()
-                || c == '-'
-                || c == '_'
-                || c == '.'
-                || c == '+'
-                || c == '?'
-                || c == '='
-            {
-                continue;
-            }
-
-            return Err(Error::InvalidName(s.to_string()));
+/// Checks a given &[u8] to match the restrictions for store path names, and
+/// returns the name as string if successful.
+pub(crate) fn validate_name(s: &[u8]) -> Result<String, Error> {
+    for c in s {
+        if c.is_ascii_alphanumeric()
+            || *c == b'-'
+            || *c == b'_'
+            || *c == b'.'
+            || *c == b'+'
+            || *c == b'?'
+            || *c == b'='
+        {
+            continue;
         }
 
-        Ok(())
+        return Err(Error::InvalidName(s.to_vec()));
     }
+
+    Ok(String::from_utf8(s.to_vec()).unwrap())
 }
 
 impl fmt::Display for StorePath {
@@ -174,8 +185,8 @@ mod tests {
     fn happy_path() {
         let example_nix_path_str =
             "00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432";
-        let nixpath =
-            StorePath::from_string(example_nix_path_str).expect("Error parsing example string");
+        let nixpath = StorePath::from_bytes(example_nix_path_str.as_bytes())
+            .expect("Error parsing example string");
 
         let expected_digest: [u8; DIGEST_SIZE] = [
             0x8a, 0x12, 0x32, 0x15, 0x22, 0xfd, 0x91, 0xef, 0xbd, 0x60, 0xeb, 0xb2, 0x48, 0x1a,
@@ -190,27 +201,27 @@ mod tests {
 
     #[test]
     fn invalid_hash_length() {
-        StorePath::from_string("00bgd045z0d4icpbc2yy-net-tools-1.60_p20170221182432")
+        StorePath::from_bytes(b"00bgd045z0d4icpbc2yy-net-tools-1.60_p20170221182432")
             .expect_err("must fail");
     }
 
     #[test]
     fn invalid_encoding_hash() {
-        StorePath::from_string("00bgd045z0d4icpbc2yyz4gx48aku4la-net-tools-1.60_p20170221182432")
+        StorePath::from_bytes(b"00bgd045z0d4icpbc2yyz4gx48aku4la-net-tools-1.60_p20170221182432")
             .expect_err("must fail");
     }
 
     #[test]
     fn more_than_just_the_bare_nix_store_path() {
-        StorePath::from_string(
-            "00bgd045z0d4icpbc2yyz4gx48aku4la-net-tools-1.60_p20170221182432/bin/arp",
+        StorePath::from_bytes(
+            b"00bgd045z0d4icpbc2yyz4gx48aku4la-net-tools-1.60_p20170221182432/bin/arp",
         )
         .expect_err("must fail");
     }
 
     #[test]
     fn no_dash_between_hash_and_name() {
-        StorePath::from_string("00bgd045z0d4icpbc2yyz4gx48ak44lanet-tools-1.60_p20170221182432")
+        StorePath::from_bytes(b"00bgd045z0d4icpbc2yyz4gx48ak44lanet-tools-1.60_p20170221182432")
             .expect_err("must fail");
     }
 
@@ -218,10 +229,11 @@ mod tests {
     fn absolute_path() {
         let example_nix_path_str =
             "00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432";
-        let nixpath_expected = StorePath::from_string(example_nix_path_str).expect("must parse");
+        let nixpath_expected =
+            StorePath::from_bytes(example_nix_path_str.as_bytes()).expect("must parse");
 
         let nixpath_actual = StorePath::from_absolute_path(
-            "/nix/store/00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432",
+            "/nix/store/00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432".as_bytes(),
         )
         .expect("must parse");
 
@@ -237,25 +249,25 @@ mod tests {
     fn absolute_path_missing_prefix() {
         assert_eq!(
             Error::MissingStoreDir(),
-            StorePath::from_absolute_path("foobar-123").expect_err("must fail")
+            StorePath::from_absolute_path(b"foobar-123").expect_err("must fail")
         );
     }
 
     #[test_case(
         "/nix/store/00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432",
-        (StorePath::from_string("00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432").unwrap(), PathBuf::new())
+        (StorePath::from_bytes(b"00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432").unwrap(), PathBuf::new())
     ; "without prefix")]
     #[test_case(
         "/nix/store/00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432/",
-        (StorePath::from_string("00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432").unwrap(), PathBuf::new())
+        (StorePath::from_bytes(b"00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432").unwrap(), PathBuf::new())
     ; "without prefix, but trailing slash")]
     #[test_case(
         "/nix/store/00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432/bin/arp",
-        (StorePath::from_string("00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432").unwrap(), PathBuf::from("bin/arp"))
+        (StorePath::from_bytes(b"00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432").unwrap(), PathBuf::from("bin/arp"))
     ; "with prefix")]
     #[test_case(
         "/nix/store/00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432/bin/arp/",
-        (StorePath::from_string("00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432").unwrap(), PathBuf::from("bin/arp/"))
+        (StorePath::from_bytes(b"00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432").unwrap(), PathBuf::from("bin/arp/"))
     ; "with prefix and trailing slash")]
     fn from_absolute_path_full(s: &str, expected: (StorePath, PathBuf)) {
         let actual = StorePath::from_absolute_path_full(s).expect("must succeed");
@@ -265,7 +277,7 @@ mod tests {
     #[test]
     fn from_absolute_path_errors() {
         assert_eq!(
-            Error::InvalidName("".into()),
+            Error::InvalidLength(),
             StorePath::from_absolute_path_full("/nix/store/").expect_err("must fail")
         );
         assert_eq!(
diff --git a/tvix/nix-compat/src/store_path/utils.rs b/tvix/nix-compat/src/store_path/utils.rs
index 2d0134cc6863..16a1bf3e1fe5 100644
--- a/tvix/nix-compat/src/store_path/utils.rs
+++ b/tvix/nix-compat/src/store_path/utils.rs
@@ -145,7 +145,7 @@ fn build_store_path_from_fingerprint_parts(
         hasher.finalize()
     };
     let compressed = compress_hash::<20>(&digest);
-    StorePath::validate_name(name)?;
+    super::validate_name(name.as_bytes())?;
     Ok(StorePath {
         digest: compressed,
         name: name.to_string(),