Skip to content

Commit 4aaad6c

Browse files
committed
Fix stale-clone-after-compaction bug in std.sort.by/sort_asc/sort_desc
native/sort.zig cloned the input array via cloneArraySlice into a raw, unrooted managed slice, then looped over it while calling arbitrary user comparator code (sort_by) or comparison helpers that can allocate. If that allocation pressure forced the allocator into its last-resort compactManagedHeap path, the clone was invisible to the compaction walk (no Object owned it yet), so its memory could be silently reused while sort.zig kept reading/writing through the now-stale slice — producing wrong sort results or a fatal VM integrity panic. This is the same stale-slice-after-compaction bug class already swept and fixed across the rest of the stdlib on 2026-07-25 (7a87570..20d2e91), apparently missed for sort.zig at the time. Fixed by allocating the working copy as a GC-visible, temp-rooted array up front via allocTempRootedManagedValueArray (the pattern array.zig already uses), re-deriving through the owning object on every access in sort_by's loop since the comparator call can compact mid-sort; sort_asc/sort_desc hold one local slice for their whole loop since their comparisons are provably allocation-free. Confirmed via git-stash A/B against the compiled CLI at --heap 128k, reproducing the crash identically pre-fix (and its absence post-fix) under plain, -Dgc_stress, and -Dheap_paranoia builds. Added a correctness regression test in compiler_test.zig; the compaction-crash itself could not be made to reproduce deterministically in-process despite matching heap size/max_objects/allocator to the CLI, so this is documented honestly in the test's comment rather than claimed as a guaranteed catch.
1 parent c0d3cca commit 4aaad6c

2 files changed

Lines changed: 115 additions & 19 deletions

File tree

src/compiler_test.zig

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,86 @@ fn expectNoTempRoots() !void {
153153
try std.testing.expectEqual(@as(usize, 0), vms.tempRootDepth());
154154
}
155155

156+
// std.sort.by (and sort_asc/sort_desc) used to clone the input array via
157+
// cloneArraySlice into a raw, unrooted managed slice, then loop over it
158+
// while calling arbitrary user comparator code (which allocates). If that
159+
// allocation pressure forced a heap compaction, the clone — invisible to
160+
// the compaction walk, since no Object owned it yet — could be silently
161+
// overwritten while native/sort.zig kept reading/writing through the now-
162+
// stale slice. Fixed by allocating the working copy as a GC-visible,
163+
// temp-rooted array up front (see native/sort.zig).
164+
//
165+
// This test is a basic correctness check for std.sort.by under GC pressure,
166+
// not a guaranteed reproduction of the compaction-corruption crash itself:
167+
// the crash was confirmed directly against the compiled CLI (`--heap 128k`,
168+
// verified via `git stash` of the fix, reproducing identically under plain,
169+
// -Dgc_stress, and -Dheap_paranoia builds — see the fix commit), but the
170+
// same script run in-process through api.Runtime here — with heap size,
171+
// max_objects, and allocator all matched to the CLI — did not reproduce it.
172+
// The corruption is apparently sensitive to something about process
173+
// environment/layout beyond these parameters; if this test class needs a
174+
// deterministic regression guard, it likely has to shell out to the actual
175+
// compiled binary rather than run in-process.
176+
test "std.sort.by does not corrupt array elements under heap pressure (compaction during a comparator call)" {
177+
var rt = try setupApiRuntime(.{
178+
.allow_io = false,
179+
.heap_size_bytes = 128 * 1024,
180+
.max_objects = 2048,
181+
});
182+
defer rt.deinit();
183+
184+
try std.testing.expect(rt.run(
185+
\\std := import("std")
186+
\\type Item struct { key int, tag string }
187+
\\func check() bool {
188+
\\ n := 20
189+
\\ arr := []
190+
\\ i := 0
191+
\\ for i < n {
192+
\\ idx := (n - i) * 37 mod 97
193+
\\ arr = std.core.append(arr, Item{ key: idx, tag: "tag-" + std.conv.to_string(idx) })
194+
\\ i = i + 1
195+
\\ }
196+
\\ pins := []
197+
\\ calls := 0
198+
\\ cmp := func(a Item, b Item) int {
199+
\\ calls = calls + 1
200+
\\ s1 := ""
201+
\\ p := 0
202+
\\ plen := 40 + (calls mod 7) * 30
203+
\\ for p < plen {
204+
\\ s1 = s1 + "m"
205+
\\ p = p + 1
206+
\\ }
207+
\\ pins = std.core.append(pins, s1)
208+
\\ if a.key < b.key { return -1 }
209+
\\ if a.key > b.key { return 1 }
210+
\\ return 0
211+
\\ }
212+
\\ sorted := std.sort.by(arr, cmp)
213+
\\ ok := true
214+
\\ k := 1
215+
\\ for k < std.core.len(sorted) {
216+
\\ if sorted[k - 1].key > sorted[k].key { ok = false }
217+
\\ k = k + 1
218+
\\ }
219+
\\ k = 0
220+
\\ for k < std.core.len(sorted) {
221+
\\ want := "tag-" + std.conv.to_string(sorted[k].key)
222+
\\ if sorted[k].tag != want { ok = false }
223+
\\ k = k + 1
224+
\\ }
225+
\\ return ok
226+
\\}
227+
) == .ok);
228+
229+
const result = rt.call("check", &.{});
230+
switch (result) {
231+
.ok => |v| try std.testing.expect(v == .boolean and v.boolean),
232+
else => return error.TestUnexpectedResult,
233+
}
234+
}
235+
156236
test "api runtime leaves no temp roots after GC-heavy success and error churn" {
157237
var rt = try setupApiRuntime(.{
158238
.allow_io = false,

src/lang/native/sort.zig

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,17 @@ pub fn dispatch(ctx: VMContext, nf: NativeFuncObj, argc: u8) !void {
1717
if (arr_val != .object) return error.TypeError;
1818
const arr_obj = arr_val.object;
1919
if (!vms.isArrayObject(arr_obj)) return error.TypeError;
20-
var items = try vms.cloneArraySlice(ctx, arr_obj);
21-
const n = items.len;
20+
const n = (try vms.asArraySlice(arr_obj)).len;
21+
// Allocate the working copy as a GC-visible, temp-rooted array
22+
// right away (rather than an owner-less clone) so it survives
23+
// any allocation below — cloneArraySlice's old unrooted slice
24+
// went stale if a later allocation forced a heap compaction.
25+
const out_arr = try vmgc.allocTempRootedManagedValueArray(ctx, n);
26+
defer ctx.vs.popTempRoot();
27+
out_arr.setAll(try vms.asArraySlice(arr_obj));
28+
// Safe to hold as one slice across the whole loop: valueGreaterThan
29+
// does no allocation, so nothing here can move out_arr's backing.
30+
const items = out_arr.obj.array_managed;
2231
if (n > 1) {
2332
var i: usize = 1;
2433
while (i < n) : (i += 1) {
@@ -30,48 +39,57 @@ pub fn dispatch(ctx: VMContext, nf: NativeFuncObj, argc: u8) !void {
3039
items[j] = key;
3140
}
3241
}
33-
const out_obj = try vmgc.vmAllocObject(ctx);
34-
out_obj.* = .{ .array_managed = items[0..n] };
3542
ctx.vs.vmPopArgs(argc);
36-
try ctx.vs.vmPush(.{ .object = out_obj });
43+
try ctx.vs.vmPush(.{ .object = out_arr.obj });
3744
},
3845
.sort_by => {
3946
const fn_val = ctx.vs.vmTop(0);
4047
const arr_val = ctx.vs.vmTop(1);
4148
if (arr_val != .object) return error.TypeError;
4249
const arr_obj = arr_val.object;
4350
if (!vms.isArrayObject(arr_obj)) return error.TypeError;
44-
var items = try vms.cloneArraySlice(ctx, arr_obj);
45-
const n = items.len;
51+
const n = (try vms.asArraySlice(arr_obj)).len;
52+
const out_arr = try vmgc.allocTempRootedManagedValueArray(ctx, n);
53+
defer ctx.vs.popTempRoot();
54+
out_arr.setAll(try vms.asArraySlice(arr_obj));
4655
if (n > 1) {
4756
var i: usize = 1;
4857
while (i < n) : (i += 1) {
49-
const key = items[i];
58+
// Re-derive out_arr's slice after every callFunction: the
59+
// comparator is arbitrary user code and can allocate,
60+
// which can trigger a heap compaction that relocates
61+
// out_arr's backing storage. A slice captured before the
62+
// call would then point at stale/reused memory.
63+
const key = out_arr.obj.array_managed[i];
5064
var j: usize = i;
5165
while (j > 0) : (j -= 1) {
52-
const cmp = try vm.callFunction(ctx, fn_val, &.{ items[j - 1], key });
66+
const prev = out_arr.obj.array_managed[j - 1];
67+
const cmp = try vm.callFunction(ctx, fn_val, &.{ prev, key });
5368
const less = if (cmp == .int) cmp.int < 0 else if (cmp == .float) cmp.float < 0 else cmp.asBool() catch {
5469
ctx.vs.setRuntimeErr("comparator must return int, float, or bool, got {s}", .{vmtyp.runtimeTypeName(cmp)});
5570
return error.TypeError;
5671
};
5772
if (less) break;
58-
items[j] = items[j - 1];
73+
out_arr.obj.array_managed[j] = out_arr.obj.array_managed[j - 1];
5974
}
60-
items[j] = key;
75+
out_arr.obj.array_managed[j] = key;
6176
}
6277
}
63-
const out_obj = try vmgc.vmAllocObject(ctx);
64-
out_obj.* = .{ .array_managed = items[0..n] };
6578
ctx.vs.vmPopArgs(argc);
66-
try ctx.vs.vmPush(.{ .object = out_obj });
79+
try ctx.vs.vmPush(.{ .object = out_arr.obj });
6780
},
6881
.sort_desc => {
6982
const arr_val = ctx.vs.vmTop(0);
7083
if (arr_val != .object) return error.TypeError;
7184
const arr_obj = arr_val.object;
7285
if (!vms.isArrayObject(arr_obj)) return error.TypeError;
73-
var items = try vms.cloneArraySlice(ctx, arr_obj);
74-
const n = items.len;
86+
const n = (try vms.asArraySlice(arr_obj)).len;
87+
const out_arr = try vmgc.allocTempRootedManagedValueArray(ctx, n);
88+
defer ctx.vs.popTempRoot();
89+
out_arr.setAll(try vms.asArraySlice(arr_obj));
90+
// Safe to hold as one slice across the whole loop: valueLessThan
91+
// does no allocation, so nothing here can move out_arr's backing.
92+
const items = out_arr.obj.array_managed;
7593
if (n > 1) {
7694
var i: usize = 1;
7795
while (i < n) : (i += 1) {
@@ -83,10 +101,8 @@ pub fn dispatch(ctx: VMContext, nf: NativeFuncObj, argc: u8) !void {
83101
items[j] = key;
84102
}
85103
}
86-
const out_obj = try vmgc.vmAllocObject(ctx);
87-
out_obj.* = .{ .array_managed = items[0..n] };
88104
ctx.vs.vmPopArgs(argc);
89-
try ctx.vs.vmPush(.{ .object = out_obj });
105+
try ctx.vs.vmPush(.{ .object = out_arr.obj });
90106
},
91107
else => {},
92108
}

0 commit comments

Comments
 (0)