Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GeneralPurposeAllocator: missing 'first free' stack trace for detected double frees #19978

Closed
squeek502 opened this issue May 15, 2024 · 1 comment · Fixed by #19987
Closed
Labels
bug Observed behavior contradicts documented or intended behavior contributor friendly This issue is limited in scope and/or knowledge of Zig internals. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@squeek502
Copy link
Collaborator

squeek502 commented May 15, 2024

Zig Version

0.13.0-dev.211+6a65561e3

Steps to Reproduce and Observed Behavior

const std = @import("std");

test "double free" {
    var gpa = std.heap.GeneralPurposeAllocator(.{
        .safety = true,
        .never_unmap = true,
        .retain_metadata = true,
    }){};
    defer std.debug.assert(gpa.deinit() == .ok);
    const allocator = gpa.allocator();

    const alloc = try allocator.alloc(u8, 8);
    allocator.free(alloc);
    allocator.free(alloc);
}

On Windows, this results in:

[gpa] (err): Double free detected. Allocation:
 First free:
 Second free:
C:\Users\Ryan\Programming\Zig\tmp\doublefree.zig:14:19: 0x7110f5 in test.double free (test.exe.obj)
    allocator.free(alloc);
                  ^
C:\Users\Ryan\Programming\Zig\zig\lib\compiler\test_runner.zig:158:25: 0x71bb90 in mainTerminal (test.exe.obj)
        if (test_fn.func()) |_| {
                        ^
C:\Users\Ryan\Programming\Zig\zig\lib\compiler\test_runner.zig:35:28: 0x711998 in main (test.exe.obj)
        return mainTerminal();
                           ^
C:\Users\Ryan\Programming\Zig\zig\lib\std\start.zig:350:53: 0x711713 in WinStartup (test.exe.obj)
    std.os.windows.ntdll.RtlExitUserProcess(callMain());
                                                    ^
???:?:?: 0x7ffe883e7343 in ??? (KERNEL32.DLL)
???:?:?: 0x7ffe896226b0 in ??? (ntdll.dll)

All 1 tests passed.
1 errors were logged.

Same thing on Linux.

Expected Behavior

The first free to have a stack trace.

@squeek502 squeek502 added bug Observed behavior contradicts documented or intended behavior standard library This issue involves writing Zig code for the standard library. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. labels May 15, 2024
@Frojdholm
Copy link
Contributor

I'm new to the Zig project, but I took a stab at investigating this.

The wrong size_class is used when fetching stack traces from empty buckets. The size_class will always be the maximum value after exhausting the search rather than the actual size_class of the allocation.

bucket.alloc_cursor is set when the bucket is added to empty_buckets and can be used to get the correct size_class.

 var size_class: usize = size_class_hint;
 const bucket = while (bucket_index < small_bucket_count) : (bucket_index += 1) {
     if (searchBucket(&self.buckets[bucket_index], @intFromPtr(old_mem.ptr), self.cur_buckets[bucket_index])) |bucket| {
         break bucket;
     }
     size_class *= 2;
 } else blk: {
     if (config.retain_metadata) {
         if (!self.large_allocations.contains(@intFromPtr(old_mem.ptr))) {
             // object not in active buckets or a large allocation, so search empty buckets
             if (searchBucket(&self.empty_buckets, @intFromPtr(old_mem.ptr), null)) |bucket| {
+                // alloc_cursor was set to slot count when bucket added to empty_buckets
+                // so we extract the correct size_class
+               size_class = @divExact(page_size, bucket.alloc_cursor);
                 // bucket is empty so is_used below will always be false and we exit there
                 break :blk bucket;
             } else {
                 @panic("Invalid free");
             }
         }
     }
     self.freeLarge(old_mem, log2_old_align, ret_addr);
     return;
 };

With this the new output from that test is:

Test [1/1] gpa_double_free.test.double free... [gpa] (err): Double free detected. Allocation:
/.../prog/zig/gpa_double_free.zig:12:38: 0x103a905 in test.double free (test)
    const alloc = try allocator.alloc(u8, 8);
                                     ^
/.../prog/zig/lib/compiler/test_runner.zig:160:25: 0x104be02 in mainTerminal (test)
        if (test_fn.func()) |_| {
                        ^
/.../prog/zig/lib/compiler/test_runner.zig:37:28: 0x103f15b in main (test)
        return mainTerminal();
                           ^
/.../prog/zig/lib/std/start.zig:514:22: 0x103b4c9 in posixCallMainAndExit (test)
            root.main();
                     ^
/.../prog/zig/lib/std/start.zig:266:5: 0x103b031 in _start (test)
    asm volatile (switch (native_arch) {
    ^
 First free:
/.../prog/zig/gpa_double_free.zig:13:19: 0x103a977 in test.double free (test)
    allocator.free(alloc);
                  ^
/.../prog/zig/lib/compiler/test_runner.zig:160:25: 0x104be02 in mainTerminal (test)
        if (test_fn.func()) |_| {
                        ^
/.../prog/zig/lib/compiler/test_runner.zig:37:28: 0x103f15b in main (test)
        return mainTerminal();
                           ^
/.../prog/zig/lib/std/start.zig:514:22: 0x103b4c9 in posixCallMainAndExit (test)
            root.main();
                     ^
/.../prog/zig/lib/std/start.zig:266:5: 0x103b031 in _start (test)
    asm volatile (switch (native_arch) {
    ^
 Second free:
/.../prog/zig/gpa_double_free.zig:14:19: 0x103a991 in test.double free (test)
    allocator.free(alloc);
                  ^
/.../prog/zig/lib/compiler/test_runner.zig:160:25: 0x104be02 in mainTerminal (test)
        if (test_fn.func()) |_| {
                        ^
/.../prog/zig/lib/compiler/test_runner.zig:37:28: 0x103f15b in main (test)
        return mainTerminal();
                           ^
/.../prog/zig/lib/std/start.zig:514:22: 0x103b4c9 in posixCallMainAndExit (test)
            root.main();
                     ^
/.../prog/zig/lib/std/start.zig:266:5: 0x103b031 in _start (test)
    asm volatile (switch (native_arch) {
    ^

All 1 tests passed.
1 errors were logged.
error: the following test command failed with exit code 1:
/.../prog/zig/zig-cache/o/7a0f61d8e085cf027ac74688c253e0f4/test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior contributor friendly This issue is limited in scope and/or knowledge of Zig internals. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants