From 41430a366f75eb7301deaca91d4aea3bbf61c8ec Mon Sep 17 00:00:00 2001 From: Erik Arvstedt Date: Mon, 12 Jun 2023 22:21:29 +0200 Subject: [PATCH 1/3] arena_allocator/reset: fix buffer overrun Previously, the buffer reserved with `retain_with_limit` was missing space for the `BufNode`. When the user-provided a limit that was smaller than `@sizeOf(BufNode)`, `reset` would store a new `BufNode` in an allocation smaller than `BufNode`, leading to a buffer overrun. --- lib/std/heap/arena_allocator.zig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/std/heap/arena_allocator.zig b/lib/std/heap/arena_allocator.zig index 9489bbb449..6f32b818d4 100644 --- a/lib/std/heap/arena_allocator.zig +++ b/lib/std/heap/arena_allocator.zig @@ -120,7 +120,7 @@ pub const ArenaAllocator = struct { } const total_size = switch (mode) { .retain_capacity => current_capacity, - .retain_with_limit => |limit| std.math.min(limit, current_capacity), + .retain_with_limit => |limit| std.math.min(@sizeOf(BufNode) + limit, current_capacity), .free_all => unreachable, }; const align_bits = std.math.log2_int(usize, @alignOf(BufNode)); From 5d3c8f4913884a4503e9f183e471b6090bb5bc92 Mon Sep 17 00:00:00 2001 From: Erik Arvstedt Date: Mon, 12 Jun 2023 22:21:30 +0200 Subject: [PATCH 2/3] arena_allocator/reset: fix use after free Previously, when the last buffer in `buffer_list` was retained after deleting all other buffers, `buffer_list` wasn't updated and pointed to a deleted buffer. --- lib/std/heap/arena_allocator.zig | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/lib/std/heap/arena_allocator.zig b/lib/std/heap/arena_allocator.zig index 6f32b818d4..632b053d90 100644 --- a/lib/std/heap/arena_allocator.zig +++ b/lib/std/heap/arena_allocator.zig @@ -139,6 +139,7 @@ pub const ArenaAllocator = struct { // reset the state before we try resizing the buffers, so we definitely have reset the arena to 0. self.state.end_index = 0; if (maybe_first_node) |first_node| { + self.state.buffer_list.first = first_node; // perfect, no need to invoke the child_allocator if (first_node.data == total_size) return true; @@ -270,3 +271,19 @@ test "ArenaAllocator (reset with preheating)" { } } } + +test "ArenaAllocator (reset while retaining a buffer)" { + var arena_allocator = ArenaAllocator.init(std.testing.allocator); + defer arena_allocator.deinit(); + const a = arena_allocator.allocator(); + + // Create two internal buffers + _ = try a.alloc(u8, 1); + _ = try a.alloc(u8, 1000); + + // Check that we have at least two buffers + try std.testing.expect(arena_allocator.state.buffer_list.first.?.next != null); + + // This retains the first allocated buffer + try std.testing.expect(arena_allocator.reset(.{ .retain_with_limit = 1 })); +} From 89bd29a9058eabf23c735761c49809bb63a68842 Mon Sep 17 00:00:00 2001 From: Erik Arvstedt Date: Mon, 12 Jun 2023 22:21:31 +0200 Subject: [PATCH 3/3] arena_allocator/reset: avoid zero-capacity allocations 1. When the arena is already empty, resetting with `retain_capacity` no longer results in allocating a buffer with zero capacity. This behavior was previously intended by the `(current_capacity == 0)` check, but wasn't correctly implemented. 2. Resetting with `.{ .retain_with_limit = 0 }` is now equivalent to `free_all` and a new buffer with zero capacity is no longer created. This is a useful side-effect of the above fixes. --- lib/std/heap/arena_allocator.zig | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/lib/std/heap/arena_allocator.zig b/lib/std/heap/arena_allocator.zig index 632b053d90..c0eeae6e61 100644 --- a/lib/std/heap/arena_allocator.zig +++ b/lib/std/heap/arena_allocator.zig @@ -108,21 +108,18 @@ pub const ArenaAllocator = struct { // Thus, only the first hand full of calls to reset() will actually need to iterate the linked // list, all future calls are just taking the first node, and only resetting the `end_index` // value. - const current_capacity = if (mode != .free_all) - @sizeOf(BufNode) + self.queryCapacity() // we need at least space for exactly one node + the current capacity - else - 0; - if (mode == .free_all or current_capacity == 0) { + const requested_capacity = switch (mode) { + .retain_capacity => self.queryCapacity(), + .retain_with_limit => |limit| std.math.min(limit, self.queryCapacity()), + .free_all => 0, + }; + if (requested_capacity == 0) { // just reset when we don't have anything to reallocate self.deinit(); self.state = State{}; return true; } - const total_size = switch (mode) { - .retain_capacity => current_capacity, - .retain_with_limit => |limit| std.math.min(@sizeOf(BufNode) + limit, current_capacity), - .free_all => unreachable, - }; + const total_size = requested_capacity + @sizeOf(BufNode); const align_bits = std.math.log2_int(usize, @alignOf(BufNode)); // Free all nodes except for the last one var it = self.state.buffer_list.first;