{"thread":{"id":"65874","subject":"[PATCH] rust: validate object map insert algorithms","startedAt":"2026-06-26T15:58:28Z","lastAt":"2026-06-26T15:58:28Z","messageCount":1,"participants":["Feng Wu via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"546484","messageId":"pull.2350.git.git.1782489506255.gitgitgadget@gmail.com","threadId":"65874","inReplyTo":null,"subject":"[PATCH] rust: validate object map insert algorithms","fromName":"Feng Wu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-06-26T15:58:26Z","receivedAt":"2026-06-26T15:58:28Z","isPatch":true,"body":"From: Feng Wu <wufengwufengwufeng@gmail.com>\n\nThe loose object map stores entries keyed by the repository's storage\nhash and the compatible hash.  ObjectMap::insert() accepts its two object\nIDs in either order, but it currently checks only whether oid1 uses the\ncompatible hash algorithm.  If it does not, oid2 is assumed to be the\ncompatible ID without validating oid2's algorithm.\n\nThat means callers can pass two IDs with the same algorithm, or an ID\nusing an unknown algorithm, and have one of them silently treated as the\nstorage ID.  This does not match the map invariant that each entry must\ncontain exactly one storage hash and one compatible hash.\n\nMake the invariant explicit by decoding both object ID algorithms and\nrejecting unknown or mismatched pairs before inserting anything.  Introduce\nObjectMapInsertError with InvalidHashAlgorithm and MismatchedAlgorithms\nvariants for clear error reporting.\n\nUpdate the existing tests to unwrap successful insertions, and add tests\nfor same-algorithm and unknown-algorithm inputs.\n\nSigned-off-by: Feng Wu <wufengwufengwufeng@gmail.com>\n---\n    rust: validate object map insert algorithms\n    \n    ObjectMap::insert() accepts a storage OID and a compatible OID in either\n    order, but it currently checks whether oid1 uses the compatible\n    algorithm, and if not, assumes oid2 is the compatible one without\n    validating oid2.\n    \n    That means inputs with two OIDs using the same hash algorithm, or an OID\n    using an unknown hash algorithm, are accepted and one of them is\n    silently treated as the storage OID. This breaks the object map\n    invariant that each entry must contain exactly one storage hash and one\n    compatible hash.\n    \n    Teach ObjectMap::insert() to decode and validate both OID algorithms\n    before inserting anything. The function now accepts only the two valid\n    permutations: (storage, compat) and (compat, storage). Unknown\n    algorithms and mismatched algorithm pairs are rejected via\n    ObjectMapInsertError.\n    \n    The tests cover successful insertion in either order, same-algorithm\n    input, and unknown-algorithm input.\n    \n    Tested with:\n    \n     * cargo fmt --all -- --check\n     * git diff --check\n     * cargo test\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2350%2Fwufengwind%2Fobject-map-insert-validation-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2350/wufengwind/object-map-insert-validation-v1\nPull-Request: https://github.com/git/git/pull/2350\n\n src/loose.rs | 79 ++++++++++++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 73 insertions(+), 6 deletions(-)\n\ndiff --git a/src/loose.rs b/src/loose.rs\nindex 24accf9c33..8f6c1fb40e 100644\n--- a/src/loose.rs\n+++ b/src/loose.rs\n@@ -510,6 +510,17 @@ pub struct ObjectMap {\n     batch: Option<ObjectMemoryMap>,\n }\n \n+#[derive(Debug, Clone, Copy, Eq, PartialEq)]\n+pub enum ObjectMapInsertError {\n+    InvalidHashAlgorithm(u32),\n+    MismatchedAlgorithms {\n+        storage: HashAlgorithm,\n+        compat: HashAlgorithm,\n+        oid1: HashAlgorithm,\n+        oid2: HashAlgorithm,\n+    },\n+}\n+\n impl ObjectMap {\n     /// Create a new `ObjectMap` with the given hash algorithms.\n     ///\n@@ -585,19 +596,39 @@ impl ObjectMap {\n     ///\n     /// If `write` is true and there is a batch started, write the object into the batch as well as\n     /// into the memory map.\n-    pub fn insert(&mut self, oid1: &ObjectID, oid2: &ObjectID, kind: MapType, write: bool) {\n-        let (compat_oid, storage_oid) =\n-            if HashAlgorithm::from_u32(oid1.algo) == Some(self.mem.compat) {\n+    pub fn insert(\n+        &mut self,\n+        oid1: &ObjectID,\n+        oid2: &ObjectID,\n+        kind: MapType,\n+        write: bool,\n+    ) -> Result<(), ObjectMapInsertError> {\n+        let oid1_algo = HashAlgorithm::from_u32(oid1.algo)\n+            .ok_or(ObjectMapInsertError::InvalidHashAlgorithm(oid1.algo))?;\n+        let oid2_algo = HashAlgorithm::from_u32(oid2.algo)\n+            .ok_or(ObjectMapInsertError::InvalidHashAlgorithm(oid2.algo))?;\n+\n+        let (storage_oid, compat_oid) =\n+            if oid1_algo == self.mem.storage && oid2_algo == self.mem.compat {\n                 (oid1, oid2)\n-            } else {\n+            } else if oid1_algo == self.mem.compat && oid2_algo == self.mem.storage {\n                 (oid2, oid1)\n+            } else {\n+                return Err(ObjectMapInsertError::MismatchedAlgorithms {\n+                    storage: self.mem.storage,\n+                    compat: self.mem.compat,\n+                    oid1: oid1_algo,\n+                    oid2: oid2_algo,\n+                });\n             };\n+\n         Self::insert_into(&mut self.mem, storage_oid, compat_oid, kind);\n         if write {\n             if let Some(ref mut batch) = self.batch {\n                 Self::insert_into(batch, storage_oid, compat_oid, kind);\n             }\n         }\n+        Ok(())\n     }\n \n     fn insert_into(\n@@ -729,9 +760,9 @@ mod tests {\n             if *swap {\n                 // Insert the item into the batch arbitrarily based on the type.  This tests that\n                 // we can specify either order and we'll do the right thing.\n-                map.insert(&s256, &s1, *kind, write);\n+                map.insert(&s256, &s1, *kind, write).unwrap();\n             } else {\n-                map.insert(&s1, &s256, *kind, write);\n+                map.insert(&s1, &s256, *kind, write).unwrap();\n             }\n         }\n \n@@ -873,6 +904,42 @@ mod tests {\n         );\n     }\n \n+    #[test]\n+    fn refuses_insert_with_mismatched_algorithms() {\n+        let mut map = ObjectMap::new(HashAlgorithm::SHA256, HashAlgorithm::SHA1);\n+        let entries = test_entries();\n+        let s256 = sha256_oid(entries[0].2);\n+        let s256_other = sha256_oid(entries[1].2);\n+        let s1 = sha1_oid(entries[0].1);\n+        let s1_other = sha1_oid(entries[1].1);\n+\n+        assert!(map.insert(&s256, &s1, MapType::LooseObject, false).is_ok());\n+        assert!(matches!(\n+            map.insert(&s256, &s256_other, MapType::LooseObject, false),\n+            Err(super::ObjectMapInsertError::MismatchedAlgorithms { .. })\n+        ));\n+        assert!(matches!(\n+            map.insert(&s1, &s1_other, MapType::LooseObject, false),\n+            Err(super::ObjectMapInsertError::MismatchedAlgorithms { .. })\n+        ));\n+    }\n+\n+    #[test]\n+    fn refuses_insert_with_unknown_algorithm() {\n+        let mut map = ObjectMap::new(HashAlgorithm::SHA256, HashAlgorithm::SHA1);\n+        let entries = test_entries();\n+        let s1 = sha1_oid(entries[0].1);\n+        let invalid_oid = ObjectID {\n+            hash: [0xffu8; 32],\n+            algo: 99,\n+        };\n+\n+        assert_eq!(\n+            map.insert(&invalid_oid, &s1, MapType::LooseObject, false),\n+            Err(super::ObjectMapInsertError::InvalidHashAlgorithm(99))\n+        );\n+    }\n+\n     #[test]\n     fn looks_up_known_oids_correctly() {\n         let map = test_map(false);\n\nbase-commit: ab776a62a78576513ee121424adb19597fbb7613\n-- \ngitgitgadget\n"}]}