Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document and restrict some unsafe code in gix-pack #1137

Merged
merged 6 commits into from
Nov 29, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
move delta Node into resolve.rs
The `Node` type is only used in `resolve.rs`, and using it safely
depends on using the child items correctly, so let's define it there.
martinvonz committed Nov 28, 2023
commit d655ee13779c40c28e518056f539e38d1b52ac8c
47 changes: 43 additions & 4 deletions gix-pack/src/cache/delta/traverse/resolve.rs
Original file line number Diff line number Diff line change
@@ -7,16 +7,55 @@ use gix_features::{progress::Progress, threading, zlib};

use crate::{
cache::delta::{
traverse::{
util::{ItemSliceSend, Node},
Context, Error,
},
traverse::{util::ItemSliceSend, Context, Error},
Item,
},
data,
data::EntryRange,
};

/// An item returned by `iter_root_chunks`, allowing access to the `data` stored alongside nodes in a [`Tree`].
pub(crate) struct Node<'a, T: Send> {
pub item: &'a mut Item<T>,
pub child_items: ItemSliceSend<'a, Item<T>>,
}

impl<'a, T: Send> Node<'a, T> {
/// Returns the offset into the pack at which the `Node`s data is located.
pub fn offset(&self) -> u64 {
self.item.offset
}

/// Returns the slice into the data pack at which the pack entry is located.
pub fn entry_slice(&self) -> crate::data::EntryRange {
self.item.offset..self.item.next_offset
}

/// Returns the node data associated with this node.
pub fn data(&mut self) -> &mut T {
&mut self.item.data
}

/// Returns true if this node has children, e.g. is not a leaf in the tree.
pub fn has_children(&self) -> bool {
!self.item.children.is_empty()
}

/// Transform this `Node` into an iterator over its children.
///
/// Children are `Node`s referring to pack entries whose base object is this pack entry.
pub fn into_child_iter(self) -> impl Iterator<Item = Node<'a, T>> + 'a {
let children = self.child_items;
// SAFETY: The index is a valid index into the children array.
// SAFETY: The resulting mutable pointer cannot be yielded by any other node.
#[allow(unsafe_code)]
self.item.children.iter().map(move |&index| Node {
item: unsafe { children.get_mut(index as usize) },
child_items: children.clone(),
})
}
}

pub(crate) struct State<'items, F, MBFN, T: Send> {
pub delta_bytes: Vec<u8>,
pub fully_resolved_delta_bytes: Vec<u8>,
44 changes: 0 additions & 44 deletions gix-pack/src/cache/delta/traverse/util.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
use std::marker::PhantomData;

use crate::cache::delta::Item;

pub(crate) struct ItemSliceSend<'a, T>
where
T: Send,
@@ -46,45 +44,3 @@ where
// SAFETY: T is `Send`, and we only ever access one T at a time. And, ptrs need that assurance, I wonder if it's always right.
#[allow(unsafe_code)]
unsafe impl<T> Send for ItemSliceSend<'_, T> where T: Send {}

/// An item returned by `iter_root_chunks`, allowing access to the `data` stored alongside nodes in a [`Tree`].
pub(crate) struct Node<'a, T: Send> {
pub item: &'a mut Item<T>,
pub child_items: ItemSliceSend<'a, Item<T>>,
}

impl<'a, T: Send> Node<'a, T> {
/// Returns the offset into the pack at which the `Node`s data is located.
pub fn offset(&self) -> u64 {
self.item.offset
}

/// Returns the slice into the data pack at which the pack entry is located.
pub fn entry_slice(&self) -> crate::data::EntryRange {
self.item.offset..self.item.next_offset
}

/// Returns the node data associated with this node.
pub fn data(&mut self) -> &mut T {
&mut self.item.data
}

/// Returns true if this node has children, e.g. is not a leaf in the tree.
pub fn has_children(&self) -> bool {
!self.item.children.is_empty()
}

/// Transform this `Node` into an iterator over its children.
///
/// Children are `Node`s referring to pack entries whose base object is this pack entry.
pub fn into_child_iter(self) -> impl Iterator<Item = Node<'a, T>> + 'a {
let children = self.child_items;
// SAFETY: The index is a valid index into the children array.
// SAFETY: The resulting mutable pointer cannot be yielded by any other node.
#[allow(unsafe_code)]
self.item.children.iter().map(move |&index| Node {
item: unsafe { children.get_mut(index as usize) },
child_items: children.clone(),
})
}
}