-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
gpa: Fix GeneralPurposeAllocator double free stack traces #19987
Conversation
Nice work! |
// 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); | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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.
d543a28
to
762e2a4
Compare
|
||
/// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be *const
Thanks! |
…aces gpa: Fix GeneralPurposeAllocator double free stack traces
Fixes #19978
I wrote the following program to test different allocation sizes