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

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.

Author: Byron

gix-object/src/tree/editor.rs

@@ -4,6 +4,7 @@ use bstr::{BStr, BString, ByteSlice, ByteVec};
 use gix_hash::ObjectId;
 use gix_hashtable::hash_map::Entry;
 use std::cmp::Ordering;
+use std::fmt::Formatter;
 
 /// A way to constrain all [tree-edits](Editor) to a given subtree.
 pub struct Cursor<'a, 'find> {
@@ -14,6 +15,26 @@ pub struct Cursor<'a, 'find> {
     prefix: BString,
 }
 
+impl std::fmt::Debug for Editor<'_> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("Editor")
+            .field("object_hash", &self.object_hash)
+            .field("path_buf", &self.path_buf)
+            .field("trees", &self.trees)
+            .finish()
+    }
+}
+
+/// The error returned by [Editor] or [Cursor] edit operation.
+#[derive(Debug, thiserror::Error)]
+#[allow(missing_docs)]
+pub enum Error {
+    #[error("Empty path components are not allowed")]
+    EmptyPathComponent,
+    #[error(transparent)]
+    FindExistingObject(#[from] crate::find::existing_object::Error),
+}
+
 /// Lifecycle
 impl<'a> Editor<'a> {
     /// Create a new editor that uses `root` as base for all edits. Use `find` to lookup existing
@@ -44,14 +65,22 @@ 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)
     }
 
     /// Remove the entry at `rela_path`, loading all trees on the path accordingly.
     /// It's no error if the entry doesn't exist, or if `rela_path` doesn't lead to an existing entry at all.
-    pub fn remove<I, C>(&mut self, rela_path: I) -> Result<&mut Self, crate::find::existing_object::Error>
+    ///
+    /// Note that trying to remove a path with an empty component is also forbidden.
+    pub fn remove<I, C>(&mut self, rela_path: I) -> Result<&mut Self, Error>
     where
         I: IntoIterator<Item = C>,
         C: AsRef<BStr>,
@@ -74,12 +103,7 @@ impl<'a> Editor<'a> {
     ///
     /// `id` can also be an empty tree, along with [the respective `kind`](EntryKind::Tree), even though that's normally not allowed
     /// in Git trees.
-    pub fn upsert<I, C>(
-        &mut self,
-        rela_path: I,
-        kind: EntryKind,
-        id: ObjectId,
-    ) -> Result<&mut Self, crate::find::existing_object::Error>
+    pub fn upsert<I, C>(&mut self, rela_path: I, kind: EntryKind, id: ObjectId) -> Result<&mut Self, Error>
     where
         I: IntoIterator<Item = C>,
         C: AsRef<BStr>,
@@ -130,22 +154,39 @@ impl<'a> Editor<'a> {
                     if tree.entries.is_empty() {
                         parent_to_adjust.entries.remove(entry_idx);
                     } else {
-                        parent_to_adjust.entries[entry_idx].oid = out(&tree)?;
+                        match out(&tree) {
+                            Ok(id) => {
+                                parent_to_adjust.entries[entry_idx].oid = id;
+                            }
+                            Err(err) => {
+                                let root_tree = parents.into_iter().next().expect("root wasn't consumed yet");
+                                self.trees.insert(path_hash(&root_tree.1), root_tree.2);
+                                return Err(err);
+                            }
+                        }
                     }
                 } else if parents.is_empty() {
                     debug_assert!(children.is_empty(), "we consume children before parents");
                     debug_assert_eq!(rela_path, self.path_buf, "this should always be the root tree");
 
                     // There may be left-over trees if they are replaced with blobs for example.
-                    let root_tree_id = out(&tree)?;
-                    match mode {
-                        WriteMode::Normal => {
-                            self.trees.clear();
+                    match out(&tree) {
+                        Ok(id) => {
+                            let root_tree_id = id;
+                            match mode {
+                                WriteMode::Normal => {
+                                    self.trees.clear();
+                                }
+                                WriteMode::FromCursor => {}
+                            }
+                            self.trees.insert(path_hash(&rela_path), tree);
+                            return Ok(root_tree_id);
+                        }
+                        Err(err) => {
+                            self.trees.insert(path_hash(&rela_path), tree);
+                            return Err(err);
                         }
-                        WriteMode::FromCursor => {}
                     }
-                    self.trees.insert(path_hash(&self.path_buf), tree);
-                    return Ok(root_tree_id);
                 } else if !tree.entries.is_empty() {
                     out(&tree)?;
                 }
@@ -161,7 +202,7 @@ impl<'a> Editor<'a> {
         &mut self,
         rela_path: I,
         kind_and_id: Option<(EntryKind, ObjectId, UpsertMode)>,
-    ) -> Result<&mut Self, crate::find::existing_object::Error>
+    ) -> Result<&mut Self, Error>
     where
         I: IntoIterator<Item = C>,
         C: AsRef<BStr>,
@@ -174,6 +215,9 @@ impl<'a> Editor<'a> {
         let new_kind_is_tree = kind_and_id.map_or(false, |(kind, _, _)| kind == EntryKind::Tree);
         while let Some(name) = rela_path.next() {
             let name = name.as_ref();
+            if name.is_empty() {
+                return Err(Error::EmptyPathComponent);
+            }
             let is_last = rela_path.peek().is_none();
             let mut needs_sorting = false;
             let current_level_must_be_tree = !is_last || new_kind_is_tree;
@@ -301,7 +345,7 @@ mod cursor {
         ///
         /// The returned cursor will then allow applying edits to the tree at `rela_path` as root.
         /// If `rela_path` is a single empty string, it is equivalent to using the current instance itself.
-        pub fn cursor_at<I, C>(&mut self, rela_path: I) -> Result<Cursor<'_, 'a>, crate::find::existing_object::Error>
+        pub fn cursor_at<I, C>(&mut self, rela_path: I) -> Result<Cursor<'_, 'a>, super::Error>
         where
             I: IntoIterator<Item = C>,
             C: AsRef<BStr>,
@@ -320,12 +364,7 @@ mod cursor {
 
     impl<'a, 'find> Cursor<'a, 'find> {
         /// Like [`Editor::upsert()`], but with the constraint of only editing in this cursor's tree.
-        pub fn upsert<I, C>(
-            &mut self,
-            rela_path: I,
-            kind: EntryKind,
-            id: ObjectId,
-        ) -> Result<&mut Self, crate::find::existing_object::Error>
+        pub fn upsert<I, C>(&mut self, rela_path: I, kind: EntryKind, id: ObjectId) -> Result<&mut Self, super::Error>
         where
             I: IntoIterator<Item = C>,
             C: AsRef<BStr>,
@@ -337,7 +376,7 @@ mod cursor {
         }
 
         /// Like [`Editor::remove()`], but with the constraint of only editing in this cursor's tree.
-        pub fn remove<I, C>(&mut self, rela_path: I) -> Result<&mut Self, crate::find::existing_object::Error>
+        pub fn remove<I, C>(&mut self, rela_path: I) -> Result<&mut Self, super::Error>
         where
             I: IntoIterator<Item = C>,
             C: AsRef<BStr>,

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());

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");
 }
 

gix-object/tests/tree/editor.rs

@@ -111,7 +111,7 @@ fn from_existing_cursor() -> crate::Result {
     odb.access_count_and_clear();
     let mut edit = gix_object::tree::Editor::new(root_tree.clone(), &odb, gix_hash::Kind::Sha1);
 
-    let mut cursor = edit.cursor_at(Some(""))?;
+    let mut cursor = edit.to_cursor();
     let actual = cursor
         .remove(Some("bin"))?
         .remove(Some("bin.d"))?
@@ -326,6 +326,56 @@ 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(["a", "\n"], EntryKind::Blob, any_blob())?
+        .write(&mut write)
+        .expect("no validation is performed");
+    assert_eq!(
+        display_tree(actual, &storage),
+        "d23290ea39c284156731188dce62c17ac6b71bda
+└── a
+    └── \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\""
+    );
+
+    let err = edit.upsert(Some(""), EntryKind::Blob, any_blob()).unwrap_err();
+    let expected_msg = "Empty path components are not allowed";
+    assert_eq!(err.to_string(), expected_msg);
+    let err = edit
+        .upsert(["fine", "", "previous is not fine"], EntryKind::Blob, any_blob())
+        .unwrap_err();
+    assert_eq!(err.to_string(), expected_msg);
+
+    let actual = edit
+        .remove(Some("a"))?
+        .remove(Some("with\0null"))?
+        .upsert(Some("works"), EntryKind::Blob, any_blob())?
+        .write(&mut write)?;
+    assert_eq!(
+        display_tree(actual, &storage),
+        "d5b913c39b06507c7c64adb16c268ce1102ef5c1
+└── works bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb.100644
+",
+        "after removing invalid entries, it can write again"
+    );
+    Ok(())
+}
+
 #[test]
 fn from_empty_add() -> crate::Result {
     let (storage, mut write, num_writes_and_clear) = new_inmemory_writes();
@@ -656,7 +706,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();
@@ -667,8 +717,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);

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,