This is an archive of the discontinued LLVM Phabricator instance.

[scudo][standalone] Fix malloc_iterate
ClosedPublic

Authored by cryptoad on Aug 14 2019, 10:18 AM.

Details

Summary

cferris's Bionic tests found an issue in Scudo's malloc_iterate.

We were inclusive of both boundaries, which resulted in a Block that
was located on said boundary to be possibly accounted for twice, or
just being accounted for while iterating on regions that are not ours
(usually the unmapped ones in between Primary regions).

The fix is to exclude the upper boundary in iterateOverChunks, and
add a regression test.

This additionally corrects a typo in a comment, and change the 64-bit
Primary iteration function to not assume that BatchClassId is 0.

Diff Detail

Repository
rL LLVM

Event Timeline

cryptoad created this revision.Aug 14 2019, 10:18 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 14 2019, 10:18 AM
Herald added subscribers: Restricted Project, delcypher. · View Herald Transcript
cryptoad updated this revision to Diff 215163.Aug 14 2019, 10:27 AM

Update the test.

hctim added inline comments.Aug 14 2019, 11:18 AM
lib/scudo/standalone/tests/wrappers_c_test.cpp
260 ↗(On Diff #215163)

BoundaryP and Counter should be zeroed in the initialisation sequence for this test to make it end-to-end.

265 ↗(On Diff #215163)

Nit: std::vector<std::unique_ptr>? to avoid manual cleanup later.

266 ↗(On Diff #215163)

From what I see, it looks like the goal of this loop is to (hopefully) get a page-aligned allocation. All the other allocations seem to not be important.

Could this test be simplified by guaranteeing a page-aligned pointer through pvalloc()?

cryptoad added inline comments.Aug 14 2019, 11:28 AM
lib/scudo/standalone/tests/wrappers_c_test.cpp
260 ↗(On Diff #215163)

Ack.

265 ↗(On Diff #215163)

Ack.

266 ↗(On Diff #215163)

Unfortunately the Block (backend) has to be aligned on a boundary (page is my choice, but it could be anything that matches the iteration), pvaloc would guarantee that the Chunk (frontend) is aligned. Hence the gymnastics with the delta.

cryptoad updated this revision to Diff 215197.Aug 14 2019, 12:16 PM
  • Additional comments to the test function
  • Moving the initialization of the static variables within the test function
cryptoad marked 2 inline comments as done.Aug 14 2019, 12:17 PM

Is this good from the reviewers perspective?

hctim added inline comments.Aug 19 2019, 2:15 PM
lib/scudo/standalone/tests/wrappers_c_test.cpp
266 ↗(On Diff #215163)

I see. Is there any way to guarantee that a Block is page-aligned without trial-and-error? If we set SpecialSize = PageSize - BlockDelta, would this guarantee this?

cryptoad added inline comments.Aug 19 2019, 2:34 PM
lib/scudo/standalone/tests/wrappers_c_test.cpp
266 ↗(On Diff #215163)

This is indeed the case if we have a class size of PageSize, then all allocations of PageSize - BlockDelta would end up in it.
It's the case for the default one, I can go with it, and if the SizeClassMap ever changes, add a bailing condition.

cryptoad updated this revision to Diff 215997.Aug 19 2019, 2:57 PM

Following Mitch's suggestion, modify the test to use a
PageSize - BlockDelta sized chunk, that in all likelihood will be
backed by a page-aligned block (dependent on the SizeClassMap but
works with our default).

Double-checked that this was returning a Count of 2 pre-patch, now 1.

cryptoad marked 4 inline comments as done.Aug 19 2019, 2:58 PM
hctim accepted this revision.Aug 19 2019, 3:09 PM

LGTM. Thanks :)

This revision is now accepted and ready to land.Aug 19 2019, 3:09 PM
This revision was automatically updated to reflect the committed changes.