Page MenuHomePhabricator

[scudo][standalone] Fix Secondary bug w/ freelist
ClosedPublic

Authored by cryptoad on Oct 31 2019, 10:44 AM.

Details

Summary

cferris@ found an issue due to the new Secondary free list behavior
and unfortunately it's completely my fault. The issue is twofold:

  • I lost track of the (major) fact that the Combined assumes that all chunks returned by the Secondary are zero'd out apprioriately when dealing with ZeroContents. With the introduction of the freelist, it's no longer the case as there can be a small portion of memory between the header and the next page boundary that is left untouched (the rest is zero'd via release). So the next time that block is returned, it's not fully zero'd out.
  • There was no test that would exercise that behavior :(

There are several ways to fix this, the one I chose makes the most
sense to me: we pass ZeroContents to the Secondary's allocate
and it zero's out the block if requested and it's coming from the
freelist. The prevents an extraneous memset in case the block
comes from map. Another possbility could have been to memset
in deallocate, but it's probably overzealous as all secondary
blocks don't need to be zero'd out.

Add a test that would have found the issue prior to fix.

Diff Detail

Event Timeline

cryptoad created this revision.Oct 31 2019, 10:44 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 31 2019, 10:44 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
cryptoad updated this revision to Diff 227318.Oct 31 2019, 11:31 AM

Minor changes.

morehouse added inline comments.Oct 31 2019, 11:33 AM
compiler-rt/lib/scudo/standalone/secondary.h
141

We currently release unconditionally in deallocate. Does this zero-out the memory?

If so, we might only need to zero the first page here, not the whole allocation.

cryptoad added inline comments.Oct 31 2019, 11:43 AM
compiler-rt/lib/scudo/standalone/secondary.h
141

You are correct, we could technically memset only the portion before the first page boundary.
As you also said it's the current as I want to move the release to abide by release_to_os_interval_ms
This would mean that eventually it could not have been released prior to reallocation.
At this point I'd rather not take a shortcut because I feel it's going to come back to me later on.

morehouse accepted this revision.Oct 31 2019, 11:51 AM
morehouse added inline comments.
compiler-rt/lib/scudo/standalone/secondary.h
141

Sounds good as long as the memset doesn't negate the perf improvemnt from using a free list.

This revision is now accepted and ready to land.Oct 31 2019, 11:51 AM
This revision was automatically updated to reflect the committed changes.