Page MenuHomePhabricator

[scudo] 32-bit quarantine sizes adjustments and bug fixes
ClosedPublic

Authored by cryptoad on Jan 31 2017, 11:27 AM.

Details

Summary

The local and global quarantine sizes were not offering a distinction for
32-bit and 64-bit platforms. This is addressed with lower values for 32-bit.

When writing additional tests for the quarantine, it was discovered that when
calling some of the allocator interface function prior to any allocation
operation having occured, the test would crash due to the allocator not being
initialized. This was addressed by making sure the allocator is initialized
for those scenarios.

Relevant tests were added in interface.cpp and quarantine.cpp.

Last change being the removal of the extraneous link dependencies for the
tests thanks to rL293220, anf the addition of the gc-sections linker flag.

Event Timeline

cryptoad created this revision.Jan 31 2017, 11:27 AM
cryptoad updated this revision to Diff 86491.Jan 31 2017, 1:52 PM

Test update.

alekseyshl requested changes to this revision.Feb 3 2017, 9:28 AM
alekseyshl added inline comments.
lib/scudo/scudo_flags.cpp
71

So, scudo default quarantine size is less than asan's one? Is there a reason?

test/scudo/quarantine.cpp
55

Wouldn't assert(CONDITION); be more debug friendly than if (!CONDITION) return 1; throughout? Should any of those tests break, to figure out what exactly went wrong, one will have to either step through the code or add printfs/asserts anyway.

This revision now requires changes to proceed.Feb 3 2017, 9:28 AM
cryptoad added inline comments.Feb 3 2017, 9:36 AM
lib/scudo/scudo_flags.cpp
71

The main reason being that the additional memory footprint incurred by the allocator should remain acceptable.
The ASan default of 256MB is, in my opinion, somewhat crippling. We just want the chunks to not be available right away, but if an attacker has the ability to allocate 64MB to get his chunk back, then he'll likely be able to allocate 256MB as well (to be faire 64MB is arbitrary, it could be less, but it feels like a decent number).

test/scudo/quarantine.cpp
55

Good point, I will change those.

cryptoad updated this revision to Diff 86984.Feb 3 2017, 10:37 AM
cryptoad edited edge metadata.

Update the tests to use asserts in error path to make failures clearer.

cryptoad updated this revision to Diff 86987.Feb 3 2017, 11:05 AM

Ensure that NDEBUG is undefined via the -UNDEBUG command line flag.

This revision is now accepted and ready to land.Feb 3 2017, 11:24 AM
cryptoad closed this revision.Feb 3 2017, 1:00 PM