refactor(tvix/castore): have SymlinkTarget-specific errors

Don't use ValidateNodeError, but SymlinkTargetError.

Also, add checks for too long symlink targets.

Change-Id: I4b533325d494232ff9d0b3f4f695f5a1a0a36199
Reviewed-on: https://cl.tvl.fyi/c/depot/+/12230
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: edef <edef@edef.eu>
Reviewed-by: Ilan Joselevich <personal@ilanjoselevich.com>
Tested-by: BuildkiteCI

tvix/castore/src/errors.rs

@@ -3,7 +3,10 @@ use thiserror::Error;
 use tokio::task::JoinError;
 use tonic::Status;
 
-use crate::path::{PathComponent, PathComponentError};
+use crate::{
+    path::{PathComponent, PathComponentError},
+    SymlinkTargetError,
+};
 
 /// Errors related to communication with the store.
 #[derive(Debug, Error, PartialEq)]
@@ -22,8 +25,8 @@ pub enum ValidateNodeError {
     #[error("invalid digest length: {0}")]
     InvalidDigestLen(usize),
     /// Invalid symlink target
-    #[error("Invalid symlink target: {}", .0.as_bstr())]
+    #[error("Invalid symlink target: {0}")]
-    InvalidSymlinkTarget(bytes::Bytes),
+    InvalidSymlinkTarget(SymlinkTargetError),
 }
 
 impl From<crate::digests::Error> for ValidateNodeError {

tvix/castore/src/import/mod.rs

@@ -6,7 +6,7 @@
 
 use crate::directoryservice::{DirectoryPutter, DirectoryService};
 use crate::path::{Path, PathBuf};
-use crate::{B3Digest, Directory, Node};
+use crate::{B3Digest, Directory, Node, SymlinkTargetError};
 use futures::{Stream, StreamExt};
 use tracing::Level;
 
@@ -91,10 +91,10 @@ where
             }
             IngestionEntry::Symlink { ref target, .. } => Node::Symlink {
                 target: bytes::Bytes::copy_from_slice(target).try_into().map_err(
-                    |e: crate::ValidateNodeError| {
+                    |e: SymlinkTargetError| {
                         IngestionError::UploadDirectoryError(
                             entry.path().to_owned(),
-                            crate::Error::StorageError(e.to_string()),
+                            crate::Error::StorageError(format!("invalid symlink target: {}", e)),
                         )
                     },
                 )?,

tvix/castore/src/nodes/mod.rs

@@ -4,7 +4,7 @@ mod symlink_target;
 
 use crate::B3Digest;
 pub use directory::Directory;
-pub use symlink_target::SymlinkTarget;
+pub use symlink_target::{SymlinkTarget, SymlinkTargetError};
 
 /// A Node is either a [DirectoryNode], [FileNode] or [SymlinkNode].
 /// Nodes themselves don't have names, what gives them names is either them

tvix/castore/src/nodes/symlink_target.rs

@@ -1,6 +1,3 @@
-// TODO: split out this error
-use crate::ValidateNodeError;
-
 use bstr::ByteSlice;
 use std::fmt::{self, Debug, Display};
 
@@ -13,6 +10,10 @@ pub struct SymlinkTarget {
     inner: bytes::Bytes,
 }
 
+/// The maximum length a symlink target can have.
+/// Linux allows 4095 bytes here.
+pub const MAX_TARGET_LEN: usize = 4095;
+
 impl AsRef<[u8]> for SymlinkTarget {
     fn as_ref(&self) -> &[u8] {
         self.inner.as_ref()
@@ -25,12 +26,28 @@ impl From<SymlinkTarget> for bytes::Bytes {
     }
 }
 
+fn validate_symlink_target<B: AsRef<[u8]>>(symlink_target: B) -> Result<B, SymlinkTargetError> {
+    let v = symlink_target.as_ref();
+
+    if v.is_empty() {
+        return Err(SymlinkTargetError::Empty);
+    }
+    if v.len() > MAX_TARGET_LEN {
+        return Err(SymlinkTargetError::TooLong);
+    }
+    if v.contains(&0x00) {
+        return Err(SymlinkTargetError::Null);
+    }
+
+    Ok(symlink_target)
+}
+
 impl TryFrom<bytes::Bytes> for SymlinkTarget {
-    type Error = ValidateNodeError;
+    type Error = SymlinkTargetError;
 
     fn try_from(value: bytes::Bytes) -> Result<Self, Self::Error> {
-        if value.is_empty() || value.contains(&b'\0') {
+        if let Err(e) = validate_symlink_target(&value) {
-            return Err(ValidateNodeError::InvalidSymlinkTarget(value));
+            return Err(SymlinkTargetError::Convert(value, Box::new(e)));
         }
 
         Ok(Self { inner: value })
@@ -38,13 +55,11 @@ impl TryFrom<bytes::Bytes> for SymlinkTarget {
 }
 
 impl TryFrom<&'static [u8]> for SymlinkTarget {
-    type Error = ValidateNodeError;
+    type Error = SymlinkTargetError;
 
     fn try_from(value: &'static [u8]) -> Result<Self, Self::Error> {
-        if value.is_empty() || value.contains(&b'\0') {
+        if let Err(e) = validate_symlink_target(&value) {
-            return Err(ValidateNodeError::InvalidSymlinkTarget(
+            return Err(SymlinkTargetError::Convert(value.into(), Box::new(e)));
-                bytes::Bytes::from_static(value),
-            ));
         }
 
         Ok(Self {
@@ -54,12 +69,13 @@ impl TryFrom<&'static [u8]> for SymlinkTarget {
 }
 
 impl TryFrom<&str> for SymlinkTarget {
-    type Error = ValidateNodeError;
+    type Error = SymlinkTargetError;
 
     fn try_from(value: &str) -> Result<Self, Self::Error> {
-        if value.is_empty() {
+        if let Err(e) = validate_symlink_target(value) {
-            return Err(ValidateNodeError::InvalidSymlinkTarget(
+            return Err(SymlinkTargetError::Convert(
-                bytes::Bytes::copy_from_slice(value.as_bytes()),
+                value.to_owned().into(),
+                Box::new(e),
             ));
         }
 
@@ -80,3 +96,128 @@ impl Display for SymlinkTarget {
         Display::fmt(self.inner.as_bstr(), f)
     }
 }
+
+/// Errors created when constructing / converting to [SymlinkTarget].
+#[derive(Debug, PartialEq, Eq, thiserror::Error)]
+#[cfg_attr(test, derive(Clone))]
+pub enum SymlinkTargetError {
+    #[error("cannot be empty")]
+    Empty,
+    #[error("cannot contain null bytes")]
+    Null,
+    #[error("cannot be over {} bytes long", MAX_TARGET_LEN)]
+    TooLong,
+    #[error("unable to convert '{:?}", .0.as_bstr())]
+    Convert(bytes::Bytes, Box<Self>),
+}
+
+#[cfg(test)]
+mod tests {
+    use bytes::Bytes;
+    use rstest::rstest;
+
+    use super::validate_symlink_target;
+    use super::{SymlinkTarget, SymlinkTargetError};
+
+    #[rstest]
+    #[case::empty(b"", SymlinkTargetError::Empty)]
+    #[case::null(b"foo\0", SymlinkTargetError::Null)]
+    fn errors(#[case] v: &'static [u8], #[case] err: SymlinkTargetError) {
+        {
+            assert_eq!(
+                Err(err.clone()),
+                validate_symlink_target(v),
+                "validate_symlink_target must fail as expected"
+            );
+        }
+
+        let exp_err_v = Bytes::from_static(v);
+
+        // Bytes
+        {
+            let v = Bytes::from_static(v);
+            assert_eq!(
+                Err(SymlinkTargetError::Convert(
+                    exp_err_v.clone(),
+                    Box::new(err.clone())
+                )),
+                SymlinkTarget::try_from(v),
+                "conversion must fail as expected"
+            );
+        }
+        // &[u8]
+        {
+            assert_eq!(
+                Err(SymlinkTargetError::Convert(
+                    exp_err_v.clone(),
+                    Box::new(err.clone())
+                )),
+                SymlinkTarget::try_from(v),
+                "conversion must fail as expected"
+            );
+        }
+        // &str, if this is valid UTF-8
+        {
+            if let Ok(v) = std::str::from_utf8(v) {
+                assert_eq!(
+                    Err(SymlinkTargetError::Convert(
+                        exp_err_v.clone(),
+                        Box::new(err.clone())
+                    )),
+                    SymlinkTarget::try_from(v),
+                    "conversion must fail as expected"
+                );
+            }
+        }
+    }
+
+    #[test]
+    fn error_toolong() {
+        assert_eq!(
+            Err(SymlinkTargetError::TooLong),
+            validate_symlink_target("X".repeat(5000).into_bytes().as_slice())
+        )
+    }
+
+    #[rstest]
+    #[case::boring(b"aa")]
+    #[case::dot(b".")]
+    #[case::dotsandslashes(b"./..")]
+    #[case::dotdot(b"..")]
+    #[case::slashes(b"a/b")]
+    #[case::slashes_and_absolute(b"/a/b")]
+    #[case::invalid_utf8(b"\xc5\xc4\xd6")]
+    fn success(#[case] v: &'static [u8]) {
+        let exp = SymlinkTarget { inner: v.into() };
+
+        // Bytes
+        {
+            let v: Bytes = v.into();
+            assert_eq!(
+                Ok(exp.clone()),
+                SymlinkTarget::try_from(v),
+                "conversion must succeed"
+            )
+        }
+
+        // &[u8]
+        {
+            assert_eq!(
+                Ok(exp.clone()),
+                SymlinkTarget::try_from(v),
+                "conversion must succeed"
+            )
+        }
+
+        // &str, if this is valid UTF-8
+        {
+            if let Ok(v) = std::str::from_utf8(v) {
+                assert_eq!(
+                    Ok(exp.clone()),
+                    SymlinkTarget::try_from(v),
+                    "conversion must succeed"
+                )
+            }
+        }
+    }
+}

tvix/castore/src/proto/mod.rs

@@ -223,10 +223,12 @@ impl Node {
                 let name: PathComponent = n.name.try_into().map_err(DirectoryError::InvalidName)?;
 
                 let node = crate::Node::Symlink {
-                    target: n
+                    target: n.target.try_into().map_err(|e| {
-                        .target
+                        DirectoryError::InvalidNode(
-                        .try_into()
+                            name.clone(),
-                        .map_err(|e| DirectoryError::InvalidNode(name.clone(), e))?,
+                            crate::ValidateNodeError::InvalidSymlinkTarget(e),
+                        )
+                    })?,
                 };
 
                 Ok((name, node))