more safety around recursion and invariants when resolving ref-deltas (#301)

Author: Byron

git-odb/src/store_impls/dynamic/find.rs

@@ -23,6 +23,20 @@ mod error {
         LoadIndex(#[from] crate::store::load_index::Error),
         #[error(transparent)]
         LoadPack(#[from] std::io::Error),
+        #[error("Object {} referred to its base object {} by its id but it's not within a multi-index", .id, .base_id)]
+        ThinPackAtRest {
+            /// the id of the base object which lived outside of the multi-index
+            base_id: git_hash::ObjectId,
+            /// The original object to lookup
+            id: git_hash::ObjectId,
+        },
+        #[error("Reached recursion limit of {} while resolving ref delta bases for {}", .max_depth, .id)]
+        DeltaBaseRecursionLimit {
+            /// the maximum recursion depth we encountered.
+            max_depth: usize,
+            /// The original object to lookup
+            id: git_hash::ObjectId,
+        },
         #[error("The base object {} could not be found but is required to decode {}", .base_id, .id)]
         DeltaBaseMissing {
             /// the id of the base object which failed to lookup
@@ -41,6 +55,25 @@ mod error {
         },
     }
 
+    #[derive(Copy, Clone)]
+    pub(crate) struct DeltaBaseRecursion<'a> {
+        pub depth: usize,
+        pub original_id: &'a git_hash::oid,
+    }
+
+    impl<'a> DeltaBaseRecursion<'a> {
+        pub fn new(id: &'a git_hash::oid) -> Self {
+            Self {
+                original_id: id,
+                depth: 0,
+            }
+        }
+        pub fn inc_depth(mut self) -> Self {
+            self.depth += 1;
+            self
+        }
+    }
+
     #[cfg(test)]
     mod tests {
         use super::*;
@@ -135,11 +168,20 @@ where
 
     fn try_find_cached_inner<'a>(
         &self,
-        id: &oid,
+        id: &git_hash::oid,
         buffer: &'a mut Vec<u8>,
         pack_cache: &mut impl DecodeEntry,
         snapshot: &mut load_index::Snapshot,
+        recursion: Option<error::DeltaBaseRecursion<'_>>,
     ) -> Result<Option<(Data<'a>, Option<Location>)>, Error> {
+        if let Some(r) = recursion {
+            if r.depth >= self.max_recursion_depth {
+                return Err(Error::DeltaBaseRecursionLimit {
+                    max_depth: self.max_recursion_depth,
+                    id: r.original_id.to_owned(),
+                });
+            }
+        }
         'outer: loop {
             {
                 let marker = snapshot.marker;
@@ -197,15 +239,33 @@ where
                                     entry_size: r.compressed_size + header_size,
                                 }),
                             )),
-                            Err(git_pack::data::decode_entry::Error::DeltaBaseUnresolved(base_id)) => {
+                            Err(git_pack::data::decode_entry::Error::DeltaBaseUnresolved(base_id))
+                                if index.is_multi_pack() =>
+                            {
+                                // Only with multi-pack indices it's allowed to jump to refer to other packs within this
+                                // multi-pack. Otherwise this would consistute a thin pack which is only allowed in transit.
+                                let _base_must_exist_in_multi_index =
+                                    index.lookup(&base_id).ok_or_else(|| Error::ThinPackAtRest {
+                                        base_id,
+                                        id: id.to_owned(),
+                                    })?;
+
                                 // special case, and we just allocate here to make it work. It's an actual delta-ref object
                                 // which is sent by some servers that points to an object outside of the pack we are looking
                                 // at right now. With the complexities of loading packs, we go into recursion here. Git itself
                                 // doesn't do a cycle check, and we won't either but limit the recursive depth.
                                 // The whole ordeal isn't efficient due to memory allocation and later mem-copying when trying again.
                                 let mut buf = Vec::new();
                                 let obj_kind = self
-                                    .try_find_cached_inner(&base_id, &mut buf, pack_cache, snapshot)
+                                    .try_find_cached_inner(
+                                        &base_id,
+                                        &mut buf,
+                                        pack_cache,
+                                        snapshot,
+                                        recursion
+                                            .map(|r| r.inc_depth())
+                                            .or_else(|| error::DeltaBaseRecursion::new(id).into()),
+                                    )
                                     .map_err(|err| Error::DeltaBaseLookup {
                                         err: Box::new(err),
                                         base_id,
@@ -343,7 +403,7 @@ where
     ) -> Result<Option<(Data<'a>, Option<Location>)>, Self::Error> {
         let id = id.as_ref();
         let mut snapshot = self.snapshot.borrow_mut();
-        self.try_find_cached_inner(id, buffer, pack_cache, &mut snapshot)
+        self.try_find_cached_inner(id, buffer, pack_cache, &mut snapshot, None)
     }
 
     fn location_by_oid(&self, id: impl AsRef<oid>, buf: &mut Vec<u8>) -> Option<Location> {

git-odb/src/store_impls/dynamic/handle.rs

@@ -114,6 +114,11 @@ pub(crate) mod index_lookup {
             })
         }
 
+        /// Returns true if this is a multi-pack index
+        pub(crate) fn is_multi_pack(&self) -> bool {
+            matches!(&self.file, handle::SingleOrMultiIndex::Multi { .. })
+        }
+
         /// Return true if the given object id exists in this index
         pub(crate) fn contains(&self, object_id: &oid) -> bool {
             match &self.file {
@@ -221,6 +226,9 @@ impl super::Store {
 
 /// Handle creation
 impl super::Store {
+    /// The amount of times a ref-delta base can be followed when multi-indices are involved.
+    pub const INITIAL_MAX_RECURSION_DEPTH: usize = 32;
+
     /// Create a new cache filled with a handle to this store, if this store is supporting shared ownership.
     ///
     /// Note that the actual type of `OwnShared` depends on the `parallel` feature toggle of the `git-features` crate.
@@ -243,6 +251,7 @@ impl super::Store {
             refresh: RefreshMode::default(),
             token: Some(token),
             snapshot: RefCell::new(self.collect_snapshot()),
+            max_recursion_depth: Self::INITIAL_MAX_RECURSION_DEPTH,
         }
     }
 
@@ -256,6 +265,7 @@ impl super::Store {
             refresh: Default::default(),
             token: Some(token),
             snapshot: RefCell::new(self.collect_snapshot()),
+            max_recursion_depth: Self::INITIAL_MAX_RECURSION_DEPTH,
         }
     }
 
@@ -336,6 +346,7 @@ where
                 .into()
             },
             snapshot: RefCell::new(self.store.collect_snapshot()),
+            max_recursion_depth: self.max_recursion_depth,
         }
     }
 }

git-odb/src/store_impls/dynamic/mod.rs

@@ -12,6 +12,12 @@ where
     pub(crate) store: S,
     /// Defines what happens when there is no more indices to load.
     pub refresh: RefreshMode,
+    /// The maximum recursion depth for resolving ref-delta base objects, that is objects referring to other objects within
+    /// a pack.
+    /// Recursive loops are possible only in purposefully crafted packs.
+    /// This value doesn't have to be huge as in typical scenarios, these kind of objects are rare and chains supposedly are
+    /// even more rare.
+    pub max_recursion_depth: usize,
 
     pub(crate) token: Option<handle::Mode>,
     snapshot: RefCell<load_index::Snapshot>,

gitoxide-core/src/index/mod.rs

@@ -211,7 +211,7 @@ pub fn checkout_exclusive(
 
     progress.done(format!(
         "Created {} {} files",
-        entries_for_checkout,
+        entries_for_checkout.saturating_sub(errors.len() + collisions.len()),
         repo.is_none().then(|| "empty").unwrap_or_default()
     ));