Merge pull request #77 from Anders429/safety

Safety audit.

.github/workflows/test.yaml

@@ -17,12 +17,14 @@ jobs:
           - stable
           - beta
           - nightly
+      fail-fast: false
     steps:
     - uses: actions/checkout@v2
     - uses: actions-rs/toolchain@v1
       with:
         toolchain: ${{ matrix.rust }}
         override: true
+        components: rust-src
     - uses: actions-rs/[email protected]
       with:
         crate: cargo-hack
@@ -172,8 +174,16 @@ jobs:
     runs-on: ubuntu-latest
     steps:
     - uses: actions/checkout@v2
+    - run: wget https://sourceware.org/pub/valgrind/valgrind-3.19.0.tar.bz2
+    - run: tar xvf valgrind-3.19.0.tar.bz2
+    - run: ./configure
+      working-directory: ./valgrind-3.19.0
+    - run: make
+      working-directory: ./valgrind-3.19.0
+    - run: sudo make install
+      working-directory: ./valgrind-3.19.0
     - run: sudo apt-get update
-    - run: sudo apt-get install valgrind
+    - run: sudo apt-get install libc6-dbg
     - uses: actions-rs/toolchain@v1
       with:
         toolchain: nightly
@@ -199,7 +209,7 @@ jobs:
       with:
         toolchain: nightly
         override: true
-        components: llvm-tools-preview
+        components: llvm-tools-preview, rust-src
     - uses: actions-rs/[email protected]
       with:
         crate: cargo-llvm-cov

Cargo.toml

@@ -18,9 +18,10 @@ rayon = {version = "1.5.1", optional = true}
 serde = {version = "1.0.133", default-features = false, features = ["alloc"], optional = true}
 
 [dev-dependencies]
+claim = "0.5.0"
 rustversion = "1.0.6"
 serde_test = "1.0.133"
-trybuild = "1.0.56"
+trybuild = "1.0.61"
 
 [features]
 # TODO: Rename this to "rayon" when namespaced dependencies are stabilized in 1.60.0.

src/archetype/identifier/impl_serde.rs

@@ -18,6 +18,7 @@ where
     {
         let mut tuple = serializer.serialize_tuple((R::LEN + 7) / 8)?;
 
+        // SAFETY: The slice returned here is guaranteed to be outlived by `self`.
         for byte in unsafe { self.as_slice() } {
             tuple.serialize_element(byte)?;
         }
@@ -65,11 +66,11 @@ where
                 }
 
                 // Check that trailing bits are not set.
+                // SAFETY: `buffer` is guaranteed to have `(R::LEN + 7) / 8` elements, so this will
+                // always be within the bounds of `buffer.`
                 let byte = unsafe { buffer.get_unchecked((R::LEN + 7) / 8 - 1) };
                 let bit = R::LEN % 8;
-                if bit != 0
-                    && unsafe { buffer.get_unchecked((R::LEN + 7) / 8 - 1) } & (255 << bit) != 0
-                {
+                if bit != 0 && byte & (255 << bit) != 0 {
                     return Err(de::Error::invalid_value(
                         Unexpected::Unsigned(u64::from(*byte)),
                         &self,

src/archetype/identifier/iter.rs

@@ -1,29 +1,69 @@
 use crate::registry::Registry;
 use core::marker::PhantomData;
 
+/// An iterator over the bits of an [`Identifier`].
+///
+/// This iterator is guaranteed to return exactly `(R::LEN + 7) / 8` boolean values indicating
+/// the components within `R` that are identified.
+///
+/// [`Identifier`]: crate::archetype::identifier::Identifier
 pub struct Iter<R>
 where
     R: Registry,
 {
+    /// The [`Registry`] defining the set of valid values of this identifier.
+    ///
+    /// Each identifier must exist within a set defined by a `Registry`. This defines a space over
+    /// which each identifier can uniquely define a set of components. Each bit within the
+    /// identifier corresponds with a component in the registry.
+    ///
+    /// [`Registry`]: crate::registry::Registry
     registry: PhantomData<R>,
 
+    /// A pointer to the allocated bits.
+    ///
+    /// Note that this allocation is not owned by this struct. It is owned by an [`Identifier`] and
+    /// is invariantly guaranteed to outlive this struct.
+    ///
+    /// As iteration progresses, this pointer will move along to point to the current byte.
+    ///
+    /// [`Identifier`]: crate::archetype::identifier::Identifier
     pointer: *const u8,
 
+    /// The current byte being iterated over.
+    ///
+    /// This byte just includes the remaining bits of the current value.
     current: u8,
+    /// The current bit position.
+    ///
+    /// If this value is greater than or equal to `(R::LEN + 7) / 8`, iteration has completed.
     position: usize,
 }
 
 impl<R> Iter<R>
 where
     R: Registry,
 {
+    /// Create a new iterator for an [`Identifier`].
+    ///
+    /// # Safety
+    /// `pointer` must be a pointer to a valid `Identifier` allocation, and must be ensured to live
+    /// as long as the returned `Iter`.
+    ///
+    /// [`Identifier`]: crate::archetype::identifier::Identifier
     pub(super) unsafe fn new(pointer: *const u8) -> Self {
         Self {
             registry: PhantomData,
 
             pointer,
 
-            current: if R::LEN > 0 { *pointer } else { 0 },
+            current: if R::LEN > 0 {
+                // SAFETY: `pointer` is a valid `Identifier` allocation that is nonempty, meaning
+                // it points to at least one `u8` which is dereferenced here.
+                unsafe { *pointer }
+            } else {
+                0
+            },
             position: 0,
         }
     }
@@ -42,8 +82,16 @@ where
             let result = self.current & 1 != 0;
             self.position += 1;
             if self.position < R::LEN && self.position % 8 == 0 {
-                self.pointer = unsafe { self.pointer.add(1) };
-                self.current = unsafe { *self.pointer };
+                self.pointer =
+                    // SAFETY: The allocation pointed to is guaranteed to have at least
+                    // `(R::LEN + 7) / 8` bytes. Therefore, since `self.position` is only
+                    // incremented once on each iteration, we will only enter this block for every
+                    // eighth byte and therefore not offset past the end of the allocation.
+                    unsafe { self.pointer.add(1) };
+                self.current =
+                    // SAFETY: Due to the reasons above, `self.pointer` will always point to a
+                    // valid `u8` that can be dereferenced here without fail.
+                    unsafe { *self.pointer };
             } else {
                 self.current >>= 1;
             }