Skip to content

Commit 06e4000

Browse files
committed
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.
1 parent 99a4c74 commit 06e4000

File tree

2 files changed

+53
-2
lines changed

2 files changed

+53
-2
lines changed

src/embedded_ring_queue.zig

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,32 @@ pub fn EmbeddedRingQueue(comptime TElement: type) type {
5252

5353
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
5454

55+
/// Returns true if the length of `storage` can be increased via `resizeNoCopy`.
56+
pub fn canResize(self: *Self) bool {
57+
if (self.len() == 0) return true;
58+
const tail_index = self.tail % self.storage.len;
59+
const head_index = self.head % self.storage.len;
60+
return tail_index > head_index;
61+
}
62+
63+
/// Replaces `storage` with `new_storage`. The caller guarantees that `new_storage`
64+
/// contains the same data as the previous storage (ie. it's the same region of
65+
/// memory but with a larger `len`, or the caller has copied the previous memory).
66+
pub fn resize(self: *Self, new_storage: []Element) void {
67+
// The backing storage can't be increased if the range of entries wraps past the end of the
68+
// backing buffer, as we'd be adding invalid entries into the middle of the queue.
69+
assert(new_storage.len >= self.storage.len and self.canResize());
70+
const prev_len = self.len();
71+
if (prev_len > 0) {
72+
self.head = self.head % self.storage.len;
73+
self.tail = self.head + prev_len;
74+
}
75+
76+
self.storage = new_storage;
77+
}
78+
79+
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
80+
5581
pub fn enqueue(self: *Self, value: Element) Error!void {
5682
if (self.enqueueIfNotFull(value)) {
5783
return;

src/pool.zig

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -532,7 +532,10 @@ pub fn Pool(
532532

533533
fn updateSlices(self: *Self) void {
534534
var slice = self._storage.slice();
535-
self._free_queue.storage = slice.items(.@"Pool._free_queue");
535+
536+
const free_queue_storage = slice.items(.@"Pool._free_queue");
537+
self._free_queue.resize(free_queue_storage);
538+
536539
self._curr_cycle = slice.items(.@"Pool._curr_cycle");
537540
inline for (column_fields, 0..) |column_field, i| {
538541
const F = column_field.type;
@@ -659,7 +662,8 @@ pub fn Pool(
659662

660663
fn didGetNewHandleNoResize(self: *Self, handle: *AddressableHandle) bool {
661664
if (self._storage.len < max_capacity and
662-
self._storage.len < self._storage.capacity)
665+
self._storage.len < self._storage.capacity and
666+
self._free_queue.canResize())
663667
{
664668
const new_index = self._storage.addOneAssumeCapacity();
665669
updateSlices(self);
@@ -1427,4 +1431,25 @@ test "Pool.setColumns() calls ColumnType.deinit()" {
14271431
try expectEqual(@as(u32, 6), deinit_count);
14281432
}
14291433

1434+
test "Adds and removes triggering resize" {
1435+
const TestPool = Pool(16, 16, void, struct {});
1436+
1437+
var pool = TestPool.init(std.testing.allocator);
1438+
defer pool.deinit();
1439+
1440+
var handles: std.ArrayListUnmanaged(TestPool.Handle) = .empty;
1441+
defer handles.deinit(std.testing.allocator);
1442+
1443+
for (0..16) |ix| {
1444+
for (0..5 * ix) |_| {
1445+
(try handles.addOne(std.testing.allocator)).* = try pool.add(.{});
1446+
}
1447+
1448+
for (0..3 * ix) |_| {
1449+
const handle = handles.orderedRemove(0);
1450+
try pool.remove(handle);
1451+
}
1452+
}
1453+
}
1454+
14301455
//------------------------------------------------------------------------------

0 commit comments

Comments
 (0)