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

Let GeneralPurposeAllocator retain metadata to report more double frees #8756

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

mattbork
Copy link
Contributor

Currently GeneralPurposeAllocator can only properly report double frees for small allocations from active buckets (buckets that weren't emptied at any point). Double frees for other small allocations and for all large allocations fall through to the "Invalid free" panic in resizeLarge (assuming config.safety is true). This will show a stack trace for the current free, but it can't show anything for the allocation or the first free. It also can't be recovered from, unlike properly reported double frees, and this may make debugging more onerous.

I've added a new flag, config.retain_metadata, that makes GeneralPurposeAllocator retain all empty buckets in a separate list and never remove entries from the large_allocations hash map (only mark them as freed). This way any double free of memory actually allocated from this allocator will always be able to find a set of stack traces to report. The extra storage for the empty bucket list in GeneralPurposeAllocator and the second stack trace and other bookkeeping in LargeAlloc is spent only when retain_metadata is set. Further, the performance of small allocations is not affected at all. However, because entries are never removed from large_allocations, lookup is likely to be slower as the map gets bigger. Leaving entries in the hash map was the simplest option and requires no extra allocations be done when freeing large objects, but maintaining a second hash map for dead large allocations is another option if the effect on performance is too high.

Another benefit of retaining metadata is that double frees can eventually be caught without having to use config.never_unmap and leak memory (all retained metadata is easily freed when the allocator is freed). For now, until #4298 is finished, retain_metadata should be used with never_unmap or else memsets in the allocator interface will cause segfaults. But in either case, because we're retaining metadata, the memory leaked by never_unmap can actually now be freed when the metadata is freed because we know exactly what memory was left deliberately unfreed. This should allow checking for double frees during only portions of a long running program with ballooning memory use. To that end, I added a public function flushRetainedMetadata that will free all retained metadata and, if never_unmap is set, recover the leaked memory as well. It is present only when retain_metadata is true.

I'd like to add tests for actually detecting and recovering from double frees but because the stack traces are logged immediately to stderr, I don't know if there's any way to keep a test from being marked as failing. Instead, I have a rather invasive test that just checks that metadata is kept and marked correctly after a free, and that all memory leaked by never_unmap is ultimately freed. For general stress testing, I also ran all of test-std with the std.testing.allocator modified to set retain_metadata (with and then without never_unmap set) and test_runner.zig modified to use a second GeneralPurposeAllocator as the backing_allocator for the testing allocator in order catch any leaks of metadata.

Since this is a large addition to somewhat intricate code that is also a basic element of the standard library, I'm happy to break down my changes in more detail if helpful.

@ifreund ifreund added the standard library This issue involves writing Zig code for the standard library. label May 12, 2021
@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label May 12, 2021
@andrewrk andrewrk added this to the 0.9.0 milestone May 12, 2021
@andrewrk andrewrk removed the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label May 12, 2021
@andrewrk
Copy link
Member

Oops, thought this was an issue, didn't realize you already implemented it. Very cool, will check this out later today :)

@andrewrk andrewrk removed this from the 0.9.0 milestone May 19, 2021
@Vexu Vexu merged commit 21af264 into ziglang:master Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants