From 9bfe827adedeffea591232fb713bea7b145c0add Mon Sep 17 00:00:00 2001 From: Justus Klausecker Date: Wed, 25 Mar 2026 11:20:21 +0100 Subject: [PATCH 1/3] Revert "std.heap.ArenaAllocator: Make `resize` and `free` check whether allocation is within current node more rigorously" This reverts commit 589bcb2544fed9a3454908adba4a2f97f1405e9c. The scenario presented in the reverted commit cannot actually happen. Even if there are two contiguous arena nodes N1 and N2 and the `end_index` of N1 points to somewhere in N2, a `resize` can never lead to an increase of the `end_index` of N1 since it checks whether it's `<= size` first. A `resize`/`free` *can* decrease `end_index`, but even if it is wrongly assumed that some allocation that belongs to N2 actually belongs to N1 based on the `end_index` of N1, it can only ever be decreased to the start of the buffer of N2. That's because a valid allocation of N2 logically cannot be at any lower address than N2 itself. And any point still in N2 can never also be in N1, so there's no danger of overwriting any other allocations of N1. --- lib/std/heap/ArenaAllocator.zig | 37 +++++++++++---------------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/lib/std/heap/ArenaAllocator.zig b/lib/std/heap/ArenaAllocator.zig index 3dad3afaad..c3e27c7c69 100644 --- a/lib/std/heap/ArenaAllocator.zig +++ b/lib/std/heap/ArenaAllocator.zig @@ -319,11 +319,6 @@ fn pushFreeList(arena: *ArenaAllocator, first: *Node, last: *Node) void { } } -fn sliceContainsSlice(container: []u8, slice: []u8) bool { - return @intFromPtr(slice.ptr) >= @intFromPtr(container.ptr) and - @intFromPtr(slice.ptr + slice.len) <= @intFromPtr(container.ptr + container.len); -} - fn alignedIndex(buf_ptr: [*]u8, end_index: usize, alignment: Alignment) usize { // Wrapping arithmetic to avoid overflows since `end_index` isn't bounded by // `size`. This is always ok since the max alignment in byte units is also @@ -548,17 +543,12 @@ fn resize(ctx: *anyopaque, memory: []u8, alignment: Alignment, new_len: usize, r assert(new_len > 0); const node = arena.loadFirstNode().?; - const buf = node.loadBuf(); - - if (!sliceContainsSlice(buf, memory)) { - // Not within current node. - return new_len <= memory.len; - } + const buf_ptr = @as([*]u8, @ptrCast(node)) + @sizeOf(Node); const cur_end_index = @atomicLoad(usize, &node.end_index, .monotonic); - - if (buf.ptr + cur_end_index != memory.ptr + memory.len) { - // It's not the most recent allocation, so it cannot be expanded. + if (buf_ptr + cur_end_index != memory.ptr + memory.len) { + // It's not the most recent allocation, so it cannot be expanded, + // but it's fine if they want to make it smaller. return new_len <= memory.len; } @@ -566,12 +556,15 @@ fn resize(ctx: *anyopaque, memory: []u8, alignment: Alignment, new_len: usize, r if (memory.len >= new_len) { break :new_end_index cur_end_index - (memory.len - new_len); } - if (buf.len - cur_end_index >= new_len - memory.len) { + const cur_buf_len: usize = node.loadBuf().len; + // Saturating arithmetic because `end_index` and `size` are not + // guaranteed to be in sync. + if (cur_buf_len -| cur_end_index >= new_len - memory.len) { break :new_end_index cur_end_index + (new_len - memory.len); } return false; }; - assert(buf.ptr + new_end_index == memory.ptr + new_len); + assert(buf_ptr + new_end_index == memory.ptr + new_len); return null == @cmpxchgStrong( usize, @@ -596,22 +589,16 @@ fn free(ctx: *anyopaque, memory: []u8, alignment: Alignment, ret_addr: usize) vo assert(memory.len > 0); const node = arena.loadFirstNode().?; - const buf = node.loadBuf(); - - if (!sliceContainsSlice(buf, memory)) { - // Not within current node; we cannot free it. - return; - } + const buf_ptr = @as([*]u8, @ptrCast(node)) + @sizeOf(Node); const cur_end_index = @atomicLoad(usize, &node.end_index, .monotonic); - - if (buf.ptr + cur_end_index != memory.ptr + memory.len) { + if (buf_ptr + cur_end_index != memory.ptr + memory.len) { // Not the most recent allocation; we cannot free it. return; } const new_end_index = cur_end_index - memory.len; - assert(buf.ptr + new_end_index == memory.ptr); + assert(buf_ptr + new_end_index == memory.ptr); _ = @cmpxchgStrong( usize, From 3af5f81e11e2fd88fe227b44753e3df0c4dab094 Mon Sep 17 00:00:00 2001 From: Justus Klausecker Date: Tue, 24 Mar 2026 16:32:10 +0100 Subject: [PATCH 2/3] std.heap.ArenaAllocator: fix `end_index` memory ordering This prevents a race between `alloc` and `free` where T1 receives memory from `alloc` that is semantically about to be freed by T2 and still being accessed, but the `free` is already visible to T1. Using acquire-release here guarantees that any `free` is only published after all accesses to the memory being freed have already happened. Co-authored-by: Jacob Young --- lib/std/heap/ArenaAllocator.zig | 89 +++++++++++++++++++++------------ 1 file changed, 56 insertions(+), 33 deletions(-) diff --git a/lib/std/heap/ArenaAllocator.zig b/lib/std/heap/ArenaAllocator.zig index c3e27c7c69..9a0ed4be6e 100644 --- a/lib/std/heap/ArenaAllocator.zig +++ b/lib/std/heap/ArenaAllocator.zig @@ -207,17 +207,20 @@ pub fn reset(arena: *ArenaAllocator, mode: ResetMode) bool { /// Concurrent accesses to node pointers generally have to have acquire/release /// semantics to guarantee that newly allocated notes are in a valid state when -/// being inserted into a list. Exceptions are possible, e.g. a CAS loop that +/// being inserted into a list. Exceptions are possible, e.g. a cmpxchg loop that /// never accesses the node returned on failure can use monotonic semantics on /// failure, but must still use release semantics on success to protect the node /// it's trying to push. const Node = struct { /// Only meant to be accessed indirectly via the methods supplied by this type, /// except if the node is owned by the thread accessing it. - /// Must always be an even number to accomodate `resize` bit. + /// Must always be an even number to accommodate `resize` bit. size: Size, - /// Concurrent accesses to `end_index` can be monotonic as long as its value - /// is compared to a version of `size` before using it to access memory. + /// Any increase of `end_index` has to use acquire semantics; + /// any decrease of `end_index` that invalidates (formerly) active allocations + /// has to use release semantics. + /// This guarantees that all accesses to memory that's about to be freed + /// happen-before the free is published. /// Since `size` can only grow and never shrink, memory access depending on /// any `end_index` <= any `size` can never be OOB. end_index: usize, @@ -352,10 +355,17 @@ fn alloc(ctx: *anyopaque, n: usize, alignment: Alignment, ret_addr: usize) ?[*]u // with a single cmpxchg afterwards, which may fail. const alignable = n + alignment.toByteUnits() - 1; - const end_index = @atomicRmw(usize, &node.end_index, .Add, alignable, .monotonic); + const end_index = @atomicRmw(usize, &node.end_index, .Add, alignable, .acquire); // acquire any memory that may have been freed const aligned_index = alignedIndex(buf.ptr, end_index, alignment); assert(end_index + alignable >= aligned_index + n); - _ = @cmpxchgStrong(usize, &node.end_index, end_index + alignable, aligned_index + n, .monotonic, .monotonic); + _ = @cmpxchgStrong( + usize, + &node.end_index, + end_index + alignable, + aligned_index + n, + .monotonic, // no need to release alignment padding; there's no one accessing it! + .monotonic, + ); if (aligned_index + n > buf.len) break :first_node .{ node, buf.len }; return buf[aligned_index..][0..n].ptr; @@ -377,7 +387,7 @@ fn alloc(ctx: *anyopaque, n: usize, alignment: Alignment, ret_addr: usize) ?[*]u const new_size = mem.alignForward(usize, @sizeOf(Node) + aligned_index + n, 2); if (new_size <= allocated_slice.len) { - // a `resize` or `free` call managed to sneak in and we need to + // A `resize` or `free` call managed to sneak in and we need to // guarantee that `size` is only ever increased; retry! continue :retry; } @@ -385,14 +395,16 @@ fn alloc(ctx: *anyopaque, n: usize, alignment: Alignment, ret_addr: usize) ?[*]u if (arena.child_allocator.rawResize(allocated_slice, .of(Node), new_size, @returnAddress())) { size = new_size; - if (@cmpxchgStrong( // strong because a spurious failure could result in suboptimal usage of this node + // strong because a spurious failure could result in suboptimal + // usage of this node + if (null == @cmpxchgStrong( usize, &node.end_index, end_index, aligned_index + n, + .acquire, // acquire any memory that may have been freed .monotonic, - .monotonic, - ) == null) { + )) { const new_buf = allocated_slice.ptr[0..new_size][@sizeOf(Node)..]; return new_buf[aligned_index..][0..n].ptr; } @@ -546,35 +558,45 @@ fn resize(ctx: *anyopaque, memory: []u8, alignment: Alignment, new_len: usize, r const buf_ptr = @as([*]u8, @ptrCast(node)) + @sizeOf(Node); const cur_end_index = @atomicLoad(usize, &node.end_index, .monotonic); + if (buf_ptr + cur_end_index != memory.ptr + memory.len) { // It's not the most recent allocation, so it cannot be expanded, // but it's fine if they want to make it smaller. return new_len <= memory.len; } - const new_end_index: usize = new_end_index: { - if (memory.len >= new_len) { - break :new_end_index cur_end_index - (memory.len - new_len); - } - const cur_buf_len: usize = node.loadBuf().len; - // Saturating arithmetic because `end_index` and `size` are not - // guaranteed to be in sync. - if (cur_buf_len -| cur_end_index >= new_len - memory.len) { - break :new_end_index cur_end_index + (new_len - memory.len); - } - return false; - }; - assert(buf_ptr + new_end_index == memory.ptr + new_len); + if (new_len <= memory.len) { + const new_end_index = cur_end_index - (memory.len - new_len); + assert(buf_ptr + new_end_index == memory.ptr + new_len); - return null == @cmpxchgStrong( - usize, - &node.end_index, - cur_end_index, - new_end_index, - .monotonic, - .monotonic, - ) or - new_len <= memory.len; // Shrinking allocations should always succeed. + _ = @cmpxchgStrong( + usize, + &node.end_index, + cur_end_index, + new_end_index, + .release, // release freed memory + .monotonic, + ); + return true; // Shrinking allocations should always succeed. + } + + // Saturating arithmetic because `end_index` is not guaranteed to be `<= size`. + // The allocation we're trying to resize *could* belong to a different node! + if (node.loadBuf().len -| cur_end_index >= new_len - memory.len) { + const new_end_index = cur_end_index + (new_len - memory.len); + assert(buf_ptr + new_end_index == memory.ptr + new_len); + + return null == @cmpxchgStrong( + usize, + &node.end_index, + cur_end_index, + new_end_index, + .acquire, // acquire any memory that may have been freed + .monotonic, + ); + } + + return false; } fn remap(ctx: *anyopaque, memory: []u8, alignment: Alignment, new_len: usize, ret_addr: usize) ?[*]u8 { @@ -592,6 +614,7 @@ fn free(ctx: *anyopaque, memory: []u8, alignment: Alignment, ret_addr: usize) vo const buf_ptr = @as([*]u8, @ptrCast(node)) + @sizeOf(Node); const cur_end_index = @atomicLoad(usize, &node.end_index, .monotonic); + if (buf_ptr + cur_end_index != memory.ptr + memory.len) { // Not the most recent allocation; we cannot free it. return; @@ -605,7 +628,7 @@ fn free(ctx: *anyopaque, memory: []u8, alignment: Alignment, ret_addr: usize) vo &node.end_index, cur_end_index, new_end_index, - .monotonic, + .release, // release freed memory .monotonic, ); } From 5363a81a57b669e43fc790b3318e1a02f967eb15 Mon Sep 17 00:00:00 2001 From: Justus Klausecker Date: Tue, 24 Mar 2026 16:47:01 +0100 Subject: [PATCH 3/3] std.heap.FixedBufferAllocator: fix `end_index` memory ordering This prevents a race between `alloc` and `free` where T1 receives memory from `alloc` that is semantically about to be freed by T2 and still being accessed, but the `free` is already visible to T1. Using acquire-release here guarantees that any `free` is only published after all accesses to the memory being freed have already happened. Co-authored-by: Jacob Young --- lib/std/heap/FixedBufferAllocator.zig | 59 +++++++++++++++++---------- 1 file changed, 38 insertions(+), 21 deletions(-) diff --git a/lib/std/heap/FixedBufferAllocator.zig b/lib/std/heap/FixedBufferAllocator.zig index e15875948a..dfad46289a 100644 --- a/lib/std/heap/FixedBufferAllocator.zig +++ b/lib/std/heap/FixedBufferAllocator.zig @@ -137,7 +137,14 @@ fn threadSafeAlloc(ctx: *anyopaque, n: usize, alignment: mem.Alignment, ret_addr const adjusted_index = cur_end_index + adjust_off; const new_end_index = adjusted_index + n; if (new_end_index > self.buffer.len) return null; - cur_end_index = @cmpxchgWeak(usize, &self.end_index, cur_end_index, new_end_index, .monotonic, .monotonic) orelse + cur_end_index = @cmpxchgWeak( + usize, + &self.end_index, + cur_end_index, + new_end_index, + .acquire, // acquire any memory that may have been freed + .monotonic, + ) orelse return self.buffer[adjusted_index..new_end_index].ptr; } } @@ -154,26 +161,36 @@ fn threadSafeResize(ctx: *anyopaque, memory: []u8, alignment: mem.Alignment, new return new_len <= memory.len; } - const new_end_index: usize = new_end_index: { - if (memory.len >= new_len) { - break :new_end_index cur_end_index - (memory.len - new_len); - } - if (fba.buffer.len - cur_end_index >= new_len - memory.len) { - break :new_end_index cur_end_index + (new_len - memory.len); - } - return false; - }; - assert(fba.buffer.ptr + new_end_index == memory.ptr + new_len); + if (new_len <= memory.len) { + const new_end_index = cur_end_index - (memory.len - new_len); + assert(fba.buffer.ptr + new_end_index == memory.ptr + new_len); - return null == @cmpxchgStrong( - usize, - &fba.end_index, - cur_end_index, - new_end_index, - .monotonic, - .monotonic, - ) or - new_len <= memory.len; // Shrinking allocations should always succeed. + _ = @cmpxchgStrong( + usize, + &fba.end_index, + cur_end_index, + new_end_index, + .release, // release freed memory + .monotonic, + ); + return true; // Shrinking allocations should always succeed. + } + + if (fba.buffer.len - cur_end_index >= new_len - memory.len) { + const new_end_index = cur_end_index + (new_len - memory.len); + assert(fba.buffer.ptr + new_end_index == memory.ptr + new_len); + + return null == @cmpxchgStrong( + usize, + &fba.end_index, + cur_end_index, + new_end_index, + .acquire, // acquire any memory that may have been freed + .monotonic, + ); + } + + return false; } fn threadSafeRemap(ctx: *anyopaque, memory: []u8, alignment: mem.Alignment, new_len: usize, ret_addr: usize) ?[*]u8 { @@ -201,7 +218,7 @@ fn threadSafeFree(ctx: *anyopaque, memory: []u8, alignment: mem.Alignment, ret_a &fba.end_index, cur_end_index, new_end_index, - .monotonic, + .release, // release freed memory .monotonic, ); }