fix!: Tree|TreeRef::write_to() will now assure the most basic sanity of entry names.

Byron · Byron · commit 6af2abc57c8a · 2024-09-03T21:41:21.000+02:00
Previously, it would allow null-bytes in the name which would corrupt the written tree.
Now this is forbidden.
For some reason, it disallowed newlines, but that is now allowed as validation is should
be handled on a higher level.
diff --git a/gix-object/src/tree/editor.rs b/gix-object/src/tree/editor.rs
@@ -44,6 +44,12 @@ impl<'a> Editor<'a> {
     /// Future calls to [`upsert`](Self::upsert) or similar will keep working on the last seen state of the
     /// just-written root-tree.
     /// If this is not desired, use [set_root()](Self::set_root()).
+    ///
+    /// ### Validation
+    ///
+    /// Note that no additional validation is performed to assure correctness of entry-names.
+    /// It is absolutely and intentionally possible to write out invalid trees with this method.
+    /// Higher layers are expected to perform detailed validation.
     pub fn write<E>(&mut self, out: impl FnMut(&Tree) -> Result<ObjectId, E>) -> Result<ObjectId, E> {
         self.path_buf.clear();
         self.write_at_pathbuf(out, WriteMode::Normal)
diff --git a/gix-object/src/tree/write.rs b/gix-object/src/tree/write.rs
@@ -12,8 +12,8 @@ use crate::{
 #[derive(Debug, thiserror::Error)]
 #[allow(missing_docs)]
 pub enum Error {
-    #[error("Newlines are invalid in file paths: {name:?}")]
-    NewlineInFilename { name: BString },
+    #[error("Nullbytes are invalid in file paths as they are separators: {name:?}")]
+    NullbyteInFilename { name: BString },
 }
 
 impl From<Error> for io::Error {
@@ -40,8 +40,8 @@ impl crate::WriteTo for Tree {
             out.write_all(mode.as_bytes(&mut buf))?;
             out.write_all(SPACE)?;
 
-            if filename.find_byte(b'\n').is_some() {
-                return Err(Error::NewlineInFilename {
+            if filename.find_byte(0).is_some() {
+                return Err(Error::NullbyteInFilename {
                     name: (*filename).to_owned(),
                 }
                 .into());
@@ -87,8 +87,8 @@ impl<'a> crate::WriteTo for TreeRef<'a> {
             out.write_all(mode.as_bytes(&mut buf))?;
             out.write_all(SPACE)?;
 
-            if filename.find_byte(b'\n').is_some() {
-                return Err(Error::NewlineInFilename {
+            if filename.find_byte(0).is_some() {
+                return Err(Error::NullbyteInFilename {
                     name: (*filename).to_owned(),
                 }
                 .into());
diff --git a/gix-object/tests/encode/mod.rs b/gix-object/tests/encode/mod.rs
@@ -88,6 +88,41 @@ mod commit {
 }
 
 mod tree {
+    use gix_object::tree::EntryKind;
+    use gix_object::{tree, WriteTo};
+
+    #[test]
+    fn write_to_does_not_validate() {
+        let mut tree = gix_object::Tree::empty();
+        tree.entries.push(tree::Entry {
+            mode: EntryKind::Blob.into(),
+            filename: "".into(),
+            oid: gix_hash::Kind::Sha1.null(),
+        });
+        tree.entries.push(tree::Entry {
+            mode: EntryKind::Tree.into(),
+            filename: "something\nwith\newlines\n".into(),
+            oid: gix_hash::ObjectId::empty_tree(gix_hash::Kind::Sha1),
+        });
+        tree.write_to(&mut std::io::sink())
+            .expect("write succeeds, no validation is performed");
+    }
+
+    #[test]
+    fn write_to_does_not_allow_separator() {
+        let mut tree = gix_object::Tree::empty();
+        tree.entries.push(tree::Entry {
+            mode: EntryKind::Blob.into(),
+            filename: "hi\0ho".into(),
+            oid: gix_hash::Kind::Sha1.null(),
+        });
+        let err = tree.write_to(&mut std::io::sink()).unwrap_err();
+        assert_eq!(
+            err.to_string(),
+            "Nullbytes are invalid in file paths as they are separators: \"hi\\0ho\""
+        );
+    }
+
     round_trip!(gix_object::Tree, gix_object::TreeRef, "tree/everything.tree");
 }
 
diff --git a/gix-object/tests/tree/editor.rs b/gix-object/tests/tree/editor.rs
@@ -292,6 +292,34 @@ fn from_existing_remove() -> crate::Result {
     Ok(())
 }
 
+#[test]
+fn from_empty_invalid_write() -> crate::Result {
+    let (storage, mut write, _num_writes_and_clear) = new_inmemory_writes();
+    let odb = StorageOdb::new(storage.clone());
+    let mut edit = gix_object::tree::Editor::new(Tree::default(), &odb, gix_hash::Kind::Sha1);
+
+    let actual = edit
+        .upsert(["", "\n"], EntryKind::Blob, any_blob())?
+        .write(&mut write)
+        .expect("no validation is performed");
+    assert_eq!(
+        display_tree(actual, &storage),
+        "d521ac024b650c93d8b75463f68ad49938d98198
+└── \n bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb.100644
+"
+    );
+
+    let err = edit
+        .upsert(Some("with\0null"), EntryKind::Blob, any_blob())?
+        .write(&mut write)
+        .unwrap_err();
+    assert_eq!(
+        err.to_string(),
+        "Nullbytes are invalid in file paths as they are separators: \"with\\0null\""
+    );
+    Ok(())
+}
+
 #[test]
 fn from_empty_add() -> crate::Result {
     let (storage, mut write, num_writes_and_clear) = new_inmemory_writes();
@@ -622,7 +650,7 @@ mod utils {
 
     pub(super) fn new_inmemory_writes() -> (
         TreeStore,
-        impl FnMut(&Tree) -> Result<ObjectId, std::convert::Infallible>,
+        impl FnMut(&Tree) -> Result<ObjectId, std::io::Error>,
         impl Fn() -> usize,
     ) {
         let store = TreeStore::default();
@@ -633,8 +661,7 @@ mod utils {
             let mut buf = Vec::with_capacity(512);
             move |tree: &Tree| {
                 buf.clear();
-                tree.write_to(&mut buf)
-                    .expect("write to memory can't fail and tree is valid");
+                tree.write_to(&mut buf)?;
                 let header = gix_object::encode::loose_header(gix_object::Kind::Tree, buf.len() as u64);
                 let mut hasher = gix_features::hash::hasher(gix_hash::Kind::Sha1);
                 hasher.update(&header);
diff --git a/gix-object/tests/tree/from_bytes.rs b/gix-object/tests/tree/from_bytes.rs
@@ -1,21 +1,32 @@
-use gix_object::{bstr::ByteSlice, tree, tree::EntryRef, TreeRef, TreeRefIter};
+use gix_object::{bstr::ByteSlice, tree, tree::EntryRef, Tree, TreeRef, TreeRefIter, WriteTo};
 
 use crate::{fixture_name, hex_to_id};
 
 #[test]
 fn empty() -> crate::Result {
+    let tree_ref = TreeRef::from_bytes(&[])?;
     assert_eq!(
-        TreeRef::from_bytes(&[])?,
+        tree_ref,
         TreeRef { entries: vec![] },
         "empty trees are valid despite usually rare in the wild"
     );
+
+    let mut buf = Vec::new();
+    tree_ref.write_to(&mut buf)?;
+    assert!(buf.is_empty());
+
+    buf.clear();
+    Tree::from(tree_ref).write_to(&mut buf)?;
+    assert!(buf.is_empty());
     Ok(())
 }
 
 #[test]
 fn everything() -> crate::Result {
+    let fixture = fixture_name("tree", "everything.tree");
+    let tree_ref = TreeRef::from_bytes(&fixture)?;
     assert_eq!(
-        TreeRef::from_bytes(&fixture_name("tree", "everything.tree"))?,
+        tree_ref,
         TreeRef {
             entries: vec![
                 EntryRef {
@@ -83,11 +94,8 @@ fn special_trees() -> crate::Result {
         ("special-5", 17),
     ] {
         let fixture = fixture_name("tree", &format!("{name}.tree"));
-        assert_eq!(
-            TreeRef::from_bytes(&fixture)?.entries.len(),
-            expected_entry_count,
-            "{name}"
-        );
+        let actual = TreeRef::from_bytes(&fixture)?;
+        assert_eq!(actual.entries.len(), expected_entry_count, "{name}");
         assert_eq!(
             TreeRefIter::from_bytes(&fixture).map(Result::unwrap).count(),
             expected_entry_count,
