This is an archive of the discontinued LLVM Phabricator instance.

Add red zones to BumpPtrAllocator under ASan
ClosedPublic

Authored by jordan_rose on Mar 7 2017, 3:50 PM.

Details

Reviewers
pete
kubamracek
Summary

To help catch buffer overruns, this patch changes BumpPtrAllocator to insert an extra unused byte between allocations when building with ASan. SpecificBumpPtrAllocator opts out of this behavior, since it needs to destroy its items later by walking the allocated memory.

I wrote this up while trying to catch a bug. It didn't actually catch anything new, but I didn't want it to go to waste. Do people think it's generally useful?

Diff Detail

Repository
rL LLVM

Event Timeline

jordan_rose created this revision.Mar 7 2017, 3:50 PM
pete accepted this revision.Mar 8 2017, 11:34 AM

Very cool. Justin and I looked at ASan on BumpPtrAllocator a while ago and I think this would have been useful then too. I think the reason you didn't find issues is that he fixed many of them already.

I think we could support SpecificBumpPtrAllocator but it would require walking the objects by 'sizeof(T) + RedZoneSize'. RedZoneSize itself would also have to be set to make sure we continue to have appropriate alignment. Not something you need to do now, but could be interesting in future.

So LGTM. Thanks!

This revision is now accepted and ready to land.Mar 8 2017, 11:34 AM

Thanks, Pete! Will land soon, then.

I think we could support SpecificBumpPtrAllocator but it would require walking the objects by 'sizeof(T) + RedZoneSize'. RedZoneSize itself would also have to be set to make sure we continue to have appropriate alignment. Not something you need to do now, but could be interesting in future.

FWIW, this doesn't work as is because SpecificBumpPtrAllocator lets you allocate *arrays* of objects as well as single objects, and the arrays obviously can't have red zones between elements.

pete added a comment.Mar 8 2017, 11:48 AM

Thanks, Pete! Will land soon, then.

I think we could support SpecificBumpPtrAllocator but it would require walking the objects by 'sizeof(T) + RedZoneSize'. RedZoneSize itself would also have to be set to make sure we continue to have appropriate alignment. Not something you need to do now, but could be interesting in future.

FWIW, this doesn't work as is because SpecificBumpPtrAllocator lets you allocate *arrays* of objects as well as single objects, and the arrays obviously can't have red zones between elements.

Good point. Never thought of that. Wouldn't be surprised if there's a whole bunch of users who never use arrays and could benefit, but still something to think about more.

jordan_rose closed this revision.Mar 8 2017, 2:05 PM

Committed in rL297310.