improve usage of unsafe (#1231)

Inspired by #1231 (comment) .

- make sure the type in question isn't use outside of its designated module
- improve documentation around safety to make the underlying data structure
  more obvious.

Author: Byron

gix-pack/src/cache/delta/mod.rs

@@ -17,7 +17,11 @@ pub mod traverse;
 ///
 pub mod from_offsets;
 
-/// An item stored within the [`Tree`]
+/// An item stored within the [`Tree`] whose data is stored in a pack file, identified by
+/// the offset of its first (`offset`) and last (`next_offset`) bytes.
+///
+/// It represents either a root entry, or one that relies on a base to be resolvable,
+/// alongside associated `data` `T`.
 pub struct Item<T> {
     /// The offset into the pack file at which the pack entry's data is located.
     pub offset: crate::data::Offset,

gix-pack/src/cache/delta/traverse/resolve.rs

@@ -14,7 +14,7 @@ use crate::{
     data::EntryRange,
 };
 
-mod node {
+mod root {
     use crate::cache::delta::{traverse::util::ItemSliceSync, Item};
 
     /// An item returned by `iter_root_chunks`, allowing access to the `data` stored alongside nodes in a [`Tree`].
@@ -26,7 +26,10 @@ mod node {
     impl<'a, T: Send> Node<'a, T> {
         /// SAFETY: The child_items must be unique among between users of the `ItemSliceSync`.
         #[allow(unsafe_code)]
-        pub(crate) unsafe fn new(item: &'a mut Item<T>, child_items: &'a ItemSliceSync<'a, Item<T>>) -> Self {
+        pub(in crate::cache::delta::traverse) unsafe fn new(
+            item: &'a mut Item<T>,
+            child_items: &'a ItemSliceSync<'a, Item<T>>,
+        ) -> Self {
             Node { item, child_items }
         }
     }
@@ -68,7 +71,7 @@ mod node {
     }
 }
 
-pub(crate) struct State<'items, F, MBFN, T: Send> {
+pub(in crate::cache::delta::traverse) struct State<'items, F, MBFN, T: Send> {
     pub delta_bytes: Vec<u8>,
     pub fully_resolved_delta_bytes: Vec<u8>,
     pub progress: Box<dyn Progress>,
@@ -78,7 +81,7 @@ pub(crate) struct State<'items, F, MBFN, T: Send> {
 }
 
 #[allow(clippy::too_many_arguments)]
-pub(crate) fn deltas<T, F, MBFN, E, R>(
+pub(in crate::cache::delta::traverse) fn deltas<T, F, MBFN, E, R>(
     objects: gix_features::progress::StepShared,
     size: gix_features::progress::StepShared,
     item: &mut Item<T>,
@@ -118,9 +121,9 @@ where
     // each node is a base, and its children always start out as deltas which become a base after applying them.
     // These will be pushed onto our stack until all are processed
     let root_level = 0;
-    // SAFETY: The child items are unique
+    // SAFETY: The child items are unique, as `item` is the root of a tree of dependent child items.
     #[allow(unsafe_code)]
-    let root_node = unsafe { node::Node::new(item, child_items) };
+    let root_node = unsafe { root::Node::new(item, child_items) };
     let mut nodes: Vec<_> = vec![(root_level, root_node)];
     while let Some((level, mut base)) = nodes.pop() {
         if should_interrupt.load(Ordering::Relaxed) {
@@ -240,7 +243,7 @@ fn deltas_mt<T, F, MBFN, E, R>(
     objects: gix_features::progress::StepShared,
     size: gix_features::progress::StepShared,
     progress: &dyn Progress,
-    nodes: Vec<(u16, node::Node<'_, T>)>,
+    nodes: Vec<(u16, root::Node<'_, T>)>,
     resolve: F,
     resolve_data: &R,
     modify_base: MBFN,

gix-pack/src/cache/delta/traverse/util.rs

@@ -1,6 +1,22 @@
 use std::marker::PhantomData;
 
-pub(crate) struct ItemSliceSync<'a, T>
+/// SAFETY: This type is used to allow access to a size-optimized vec of items that form a
+/// tree, and we need to access it concurrently with each thread taking its own root node,
+/// and working its way through all the reachable leaves.
+///
+/// The tree was built by decoding a pack whose entries refer to its bases only by OFS_DELTA -
+/// they are pointing backwards only which assures bases have to be listed first, and that each entry
+/// only has a single parent.
+///
+/// REF_DELTA entries aren't supported here, and cause immediate failure - they are expected to have
+/// been resolved before as part of the thin-pack handling.
+///
+/// If we somehow would allow REF_DELTA entries to point to an in-pack object, then in theory malicious packs could
+/// cause all kinds of graphs as they can point anywhere in the pack, but they still can't link an entry to
+/// more than one base. And that's what one would really have to do for two threads to encounter the same child.
+///
+/// Thus I believe it's impossible for this data structure to end up in a place where it violates its assumption.
+pub(in crate::cache::delta::traverse) struct ItemSliceSync<'a, T>
 where
     T: Send,
 {
@@ -14,7 +30,7 @@ impl<'a, T> ItemSliceSync<'a, T>
 where
     T: Send,
 {
-    pub fn new(items: &'a mut [T]) -> Self {
+    pub(in crate::cache::delta::traverse) fn new(items: &'a mut [T]) -> Self {
         ItemSliceSync {
             items: items.as_mut_ptr(),
             #[cfg(debug_assertions)]
@@ -25,7 +41,7 @@ where
 
     // SAFETY: The index must point into the slice and must not be reused concurrently.
     #[allow(unsafe_code)]
-    pub unsafe fn get_mut(&self, index: usize) -> &'a mut T {
+    pub(in crate::cache::delta::traverse) unsafe fn get_mut(&self, index: usize) -> &'a mut T {
         #[cfg(debug_assertions)]
         if index >= self.len {
             panic!("index out of bounds: the len is {} but the index is {index}", self.len);