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