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

Make tracking allocator default to crashing on a bad free instead of adding to bad_free_array #4605

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

karl-zylinski
Copy link
Contributor

@karl-zylinski karl-zylinski commented Dec 21, 2024

This makes the tracking allocator default to crashing when a bad free occurs, instead of adding the bad free to an array. The crash will look something like this:

C:/code/playground/playground.odin(25:2) Tracking allocator error: Bad free of pointer 2003974722200

Rationale

Almost once a month I talk to someone who has misbehaving code, and I ask them if they check the bad free array. They say that they use tracking allocator but didn't think of checking the bad free array as required. After all, without tracking allocator bad frees tend to crash so one finds them anyways. But with the tracking allocator, bad frees just silently pass by if you forget to check the bad_free_array.

A better default behavior is therefore for tracking allocator to also crash, but still provide all the nice source code location information it has access to.

Also, not only does the old style of adding to bad_free_array cause people to miss bad frees. It can also cause memory corruption in their development build! Let me explain why that can happen, here's the old code that added stuff to the bad_free_array if it wasn't in the allocation_map:

if mode == .Free && old_memory != nil && old_memory not_in data.allocation_map {
	append(&data.bad_free_array, Tracking_Allocator_Bad_Free_Entry{
		memory = old_memory,
		location = loc,
	})
} else {
	result = data.backing.procedure(data.backing.data, mode, size, alignment, old_memory, old_size, loc) or_return
}

If you forgot to check bad_free_array, then all the bad frees were just silently added to the list. But to make things worse: Once in a blue moon the bad free would actually be of a valid pointer that exists in data.allocation_map. In that case the else block happens in the code above: The bad free just deallocates some random memory! This can of course happen with a crash-callback too. But it is very likely that it will have crashed on some allocation before getting to one of those rare cases that ends up in the else block.

Another problem is that core and vendor has several examples that look like this:

main :: proc() {
	track := mem.Tracking_Allocator{}
	mem.tracking_allocator_init(&track, context.allocator)

	context.allocator = mem.tracking_allocator(&track)

	demo()

	if len(track.allocation_map) > 0 {
		fmt.println("Leaks:")
		for _, v in track.allocation_map {
			fmt.printf("\t%v\n\n", v)
		}
	}
}

For anyone who learned about the tracking allocator from this example, they might never have known that the bad_free_array exists. So they just have lots of silent bad frees + possible memory corruptions.

I know people would probably see these bad frees if they ever make a build without tracking allocator (because then it would probably crash on those bad frees). But I wouldn't be surprised if there are people out there who just give up on Odin because their program is just misbehaving (due to tracking-allocator-i-didnt-check-the-bad-free-array-memory-corruption), but they can't figure out why.

Backwards compatibility

This new default behavior is implemented using a callback Tracking_Allocator.bad_free_callback. This callback can be overridden to tracking_allocator_bad_free_callback_add_to_array in order to have the old behavior.

The bad_free_array is still in the tracking allocator, making sure we don't break a lot of code at once. But in most programs it will not be used any more, so the "bad free checks" on shutdown won't do anything more. I can update the overview docs etc to remove the bad_free_array checks.

There were a few tests that explicitly checked bad_free_array, for those tests I use the callback that implements the old behavior.

… add to bad_free_array. The bad_free_array remains to not break old code. The new default behavior is implemented in a callback that you can override, there's a second provided callback that provides the old behavior where an element was added to bad_free_array. Rationale: Many people are just checking the allocation_map, but don't check the bad free array. Several examples throughout core that use tracking allocator don't check bad_free_array either, so people have been taught not to check it.
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.

1 participant