From 06e400011247e72acb868186df3cdfb4dd15c701 Mon Sep 17 00:00:00 2001
From: kcbanner <kcbanner@gmail.com>
Date: Sat, 19 Apr 2025 14:45:39 -0400
Subject: [PATCH] Fix out-of-bounds panic caused by use of undefined indices
 when the free queue storage is resized.

Before this change, the following sequence of events was possible:
1. The current free queue storage region (between head and tail) wraps around the end of the array
2. An entry is added via `didGetNewHandleNoResize`, which resizes the free queue backing slice
3. The region between head and tail now contains an undefined entry

I initially fixed this by always preferring to use entries from the free list, but the problem with that solution
is that a smaller number of indices are used, and their cycle counts are incremented more often, which
would increase the chances of a stale handle accidentally aliasing a re-used handle.

This new strategy is to prefer entries from the free list only if it spans past the end of the storage buffer,
and to prefer unused entries otherwise.
---
 src/embedded_ring_queue.zig | 26 ++++++++++++++++++++++++++
 src/pool.zig                | 29 +++++++++++++++++++++++++++--
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/src/embedded_ring_queue.zig b/src/embedded_ring_queue.zig
index 93bae47..c2eddc1 100644
--- a/src/embedded_ring_queue.zig
+++ b/src/embedded_ring_queue.zig
@@ -52,6 +52,32 @@ pub fn EmbeddedRingQueue(comptime TElement: type) type {
 
         // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 
+        /// Returns true if the length of `storage` can be increased via `resizeNoCopy`.
+        pub fn canResize(self: *Self) bool {
+            if (self.len() == 0) return true;
+            const tail_index = self.tail % self.storage.len;
+            const head_index = self.head % self.storage.len;
+            return tail_index > head_index;
+        }
+
+        /// Replaces `storage` with `new_storage`. The caller guarantees that `new_storage`
+        /// contains the same data as the previous storage (ie. it's the same region of
+        /// memory but with a larger `len`, or the caller has copied the previous memory).
+        pub fn resize(self: *Self, new_storage: []Element) void {
+            // The backing storage can't be increased if the range of entries wraps past the end of the
+            // backing buffer, as we'd be adding invalid entries into the middle of the queue.
+            assert(new_storage.len >= self.storage.len and self.canResize());
+            const prev_len = self.len();
+            if (prev_len > 0) {
+                self.head = self.head % self.storage.len;
+                self.tail = self.head + prev_len;
+            }
+
+            self.storage = new_storage;
+        }
+
+        // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+
         pub fn enqueue(self: *Self, value: Element) Error!void {
             if (self.enqueueIfNotFull(value)) {
                 return;
diff --git a/src/pool.zig b/src/pool.zig
index 40849af..172d5d5 100644
--- a/src/pool.zig
+++ b/src/pool.zig
@@ -532,7 +532,10 @@ pub fn Pool(
 
         fn updateSlices(self: *Self) void {
             var slice = self._storage.slice();
-            self._free_queue.storage = slice.items(.@"Pool._free_queue");
+
+            const free_queue_storage = slice.items(.@"Pool._free_queue");
+            self._free_queue.resize(free_queue_storage);
+
             self._curr_cycle = slice.items(.@"Pool._curr_cycle");
             inline for (column_fields, 0..) |column_field, i| {
                 const F = column_field.type;
@@ -659,7 +662,8 @@ pub fn Pool(
 
         fn didGetNewHandleNoResize(self: *Self, handle: *AddressableHandle) bool {
             if (self._storage.len < max_capacity and
-                self._storage.len < self._storage.capacity)
+                self._storage.len < self._storage.capacity and
+                self._free_queue.canResize())
             {
                 const new_index = self._storage.addOneAssumeCapacity();
                 updateSlices(self);
@@ -1427,4 +1431,25 @@ test "Pool.setColumns() calls ColumnType.deinit()" {
     try expectEqual(@as(u32, 6), deinit_count);
 }
 
+test "Adds and removes triggering resize" {
+    const TestPool = Pool(16, 16, void, struct {});
+
+    var pool = TestPool.init(std.testing.allocator);
+    defer pool.deinit();
+
+    var handles: std.ArrayListUnmanaged(TestPool.Handle) = .empty;
+    defer handles.deinit(std.testing.allocator);
+
+    for (0..16) |ix| {
+        for (0..5 * ix) |_| {
+            (try handles.addOne(std.testing.allocator)).* = try pool.add(.{});
+        }
+
+        for (0..3 * ix) |_| {
+            const handle = handles.orderedRemove(0);
+            try pool.remove(handle);
+        }
+    }
+}
+
 //------------------------------------------------------------------------------