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

gpa: Fix GeneralPurposeAllocator double free stack traces #19987

Merged
merged 2 commits into from
May 20, 2024

Conversation

Frojdholm
Copy link
Contributor

@Frojdholm Frojdholm commented May 16, 2024

Fixes #19978

I wrote the following program to test different allocation sizes

const std = @import("std");

const page_size = std.mem.page_size;

pub fn main() !void {
    var gpa = std.heap.GeneralPurposeAllocator(.{
        .safety = true,
        .never_unmap = true,
        .retain_metadata = true,
    }){};
    defer std.debug.assert(gpa.deinit() == .ok);
    const allocator = gpa.allocator();

    var size_class: usize = 1;
    while (size_class <= page_size) {
        std.debug.print("allocating {}\n", .{size_class * 8});
        const alloc = try allocator.alloc(u8, size_class);
        allocator.free(alloc);
        allocator.free(alloc);
        size_class *= 2;
    }
}

@squeek502
Copy link
Collaborator

Nice work!

Comment on lines 1406 to 1410
// the metadata tracking system relies on alloc_cursor of empty buckets
// being set to the slot count so that we can get back the size class.
const empty_bucket = GPA.searchBucket(&gpa.empty_buckets, @intFromPtr(small.ptr), null).?;
try std.testing.expect(@divExact(page_size, empty_bucket.alloc_cursor) == size_class);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this test should be included in this PR, but I found it not obvious that this was the way to extract the size_class from an empty bucket. It's hidden away in a branch of the free function and if this invariant changes the metadata tracking system will break.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about adding a function to BucketHeader, something like:

/// Only valid for buckets within `empty_buckets`, and relies on the `alloc_cursor`
/// of empty buckets being set to `slot_count` when they are added to `empty_buckets`
fn sizeClassOfEmptyBucket(bucket: *BucketHeader) usize {
    // alloc_cursor was set to slot count when bucket added to empty_buckets
    // so we can get the size_class
    return @divExact(page_size, bucket.alloc_cursor);
}

and then using that everywhere this @divExact(page_size, bucket.alloc_cursor) calculation is done?

@Frojdholm Frojdholm marked this pull request as ready for review May 17, 2024 19:11
Empty buckets have their `alloc_cursor` set to `slot_count` to allow the
size class to be calculated later. This happens deep within the free
function.

This adds a helper and a test to verify that the size class of empty
buckets is indeed recoverable.
The wrong `size_class` was used when fetching stack traces from empty
buckets. The `size_class` would always be the maximum value after
exhausting the search of active buckets rather than the actual
`size_class` of the allocation.
@Frojdholm Frojdholm force-pushed the fix-gpa-double-free-stack-traces branch from d543a28 to 762e2a4 Compare May 18, 2024 09:50
@Frojdholm Frojdholm changed the title std: Fix GeneralPurposeAllocator double free stack traces gpa: Fix GeneralPurposeAllocator double free stack traces May 18, 2024

/// Only valid for buckets within `empty_buckets`, and relies on the `alloc_cursor`
/// of empty buckets being set to `slot_count` when they are added to `empty_buckets`
fn emptyBucketSizeClass(bucket: *BucketHeader) usize {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be *const

@andrewrk andrewrk merged commit 6574424 into ziglang:master May 20, 2024
10 checks passed
@andrewrk
Copy link
Member

Thanks!

@Frojdholm Frojdholm deleted the fix-gpa-double-free-stack-traces branch May 20, 2024 11:38
andrewrk added a commit that referenced this pull request May 22, 2024
…aces

gpa: Fix GeneralPurposeAllocator double free stack traces
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.

GeneralPurposeAllocator: missing 'first free' stack trace for detected double frees
3 participants