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

Junk fill freed memory on dev builds to help find use after frees. #86818

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

Conversation

tdaven
Copy link
Contributor

@tdaven tdaven commented Jan 5, 2024

While investigating #86427, not all systems would crash. My desktops with Fedora 39 crash consistently but other systems like mingw windows build did not. This makes it hard for others to confirm the issue or investigate when they can't reproduce the issue.

Filling memory with junk on free helps give deterministic behavior. Its currently guarded by DEV_ENABLED where there are already lots of slower checks being done. There is nothing magic about the 0xab value I'm filling with. It was picked as I've used it before.

With junk filling, the mingw build mentioned above consistently crashes the same as my Fedora 39 systems.

Since it was helpful for confirming behavior looking into #86427, I figured make a PR for it. I could easily see something like this not being desired (and it fairly easy to do when needed).

@tdaven tdaven requested a review from a team as a code owner January 5, 2024 03:37
@RandomShaper
Copy link
Member

I'm generally in favor of these measures. In fact, I PR'd something similar time ago (#32949), but considering it was rejected at the time, maybe we need to re-discuss it.

@Ansraer
Copy link
Contributor

Ansraer commented Jan 6, 2024

Sounds like a sensible change to me unless it has too much of a performance cost (dev builds are already quite slow).
Though are you sure that 0xAB is a good number? I think that 0xBAD or 0xBADC0DE would be better choices due to uhm... reasons. 😁

@tdaven
Copy link
Contributor Author

tdaven commented Jan 6, 2024

Using 0xab is just an easy thing. We could do some multi-byte value if desired. That just means we have to calculate how many times we can copy that value into the range (unless we are comfortable falling back on alignment requirements). You just want something that is likely to cause issues if accessed and be obvious. Zero or 0xff are less obvious, a big block of repeating value of 0xab or something else is pretty easy to spot.

For reference, jemalloc defaults to 0x5a (search for opt.junk in the manual) when enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants