Skip to content

DebugAllocator: Fix bucket removal logic causing segfault/leak #23390

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

Merged

Conversation

SuperAuguste
Copy link
Contributor

Hey Ziguanas 0/

Ran into this one while at work and it drove me crazy for a good couple hours until a coworker and I realized it was a bug in the DebugAllocator rather than in our own code!

Here's a repro; the first test will not error when it should, and the second will segfault:

const std = @import("std");

const size_class_0_slot_count = 1308;

test "Head erasure" {
    var debug_allocator: std.heap.DebugAllocator(.{}) = .init;
    defer _ = debug_allocator.deinit();

    const allocator = debug_allocator.allocator();

    // Current bucket linked list: [null]

    for (0..size_class_0_slot_count) |_| {
        _ = try allocator.create(u8);
    }

    // Current bucket linked list: [null <- bucket_0]

    const new_bucket = try allocator.create(u8);

    // Current bucket linked list: [null <- bucket_0 <- bucket_1]

    allocator.destroy(new_bucket);

    // Current bucket linked list: [null]

    // No errors despite clearly leaking memory due to our `prev` (and thus the existence of
    // bucket_0 within the DebugAllocator) being erased!
}

test "Intermediate erasure" {
    var debug_allocator: std.heap.DebugAllocator(.{}) = .init;
    defer _ = debug_allocator.deinit();

    const allocator = debug_allocator.allocator();

    // Current bucket linked list: [null]

    for (0..size_class_0_slot_count) |_| {
        _ = try allocator.create(u8);
    }

    // Current bucket linked list: [null <- bucket_0]

    var bucket_1_items: [size_class_0_slot_count]*u8 = undefined;
    for (&bucket_1_items) |*item| {
        item.* = try allocator.create(u8);
    }

    // Current bucket linked list: [null <- bucket_0 <- bucket_1]

    _ = try allocator.create(u8);

    // Current bucket linked list: [null <- bucket_0 <- bucket_1 <- bucket_2]

    for (bucket_1_items) |item| {
        allocator.destroy(item);
    }

    // Current bucket linked list: [freed memory <- bucket_2]

    // Causes a segfault on deinit!
}

My proposed patch is strictly less performant (though not significantly unless you're freeing rather old buckets on a specific size class on the regular I'd imagine) and could be made inexpensive by making the buckets doubly-linked list, though that of course has its own space tradeoffs.

Let me know if you'd like to go the doubly-linked route instead (or something else entirely). Cheers! :)

@SuperAuguste SuperAuguste changed the title DebugAllocator: Fix bucket removal logic DebugAllocator: Fix bucket removal logic causing segfault/leak Mar 28, 2025
@alexrp alexrp added this to the 0.14.1 milestone Mar 28, 2025
@squeek502
Copy link
Collaborator

squeek502 commented Mar 28, 2025

I believe using a linear search over the buckets will (partially, since it's only done in free here) reintroduce the performance problems described/fixed in #17383 (before the GPA/DebugAllocator was entirely rewritten in #20511)

Don't have any recommendations on how to avoid the linear scan yet, will try to come up with some, just wanted to mention this now. A doubly linked list seems pretty reasonable, though.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof that's a silly mistake I made, isn't it?

Thanks for the bug report.

As @squeek502 pointed out, this should be solved instead by adding a next pointer to buckets so that it is O(1) instead of O(N).

Also, this is the kind of thing I want to start finding via integrated fuzzing!

@SuperAuguste
Copy link
Contributor Author

Addressed y'alls feedback :)

@andrewrk andrewrk enabled auto-merge (squash) April 2, 2025 05:42
@andrewrk
Copy link
Member

andrewrk commented Apr 2, 2025

Thanks!

@andrewrk andrewrk merged commit bfab958 into ziglang:master Apr 2, 2025
17 of 18 checks passed
alexrp pushed a commit that referenced this pull request Apr 2, 2025
@SuperAuguste SuperAuguste deleted the fix-debug-allocator-linked-list branch April 2, 2025 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants