This is an archive of the discontinued LLVM Phabricator instance.

[scudo][standalone] Introduce the Quarantine
ClosedPublic

Authored by cryptoad on May 1 2019, 9:17 AM.

Details

Summary

The Quarantine is used to hold chunks for a little while prior to
actually releasing them for potential reuse. The code is pretty much
the same as the sanitizer_common one, with additional shuffling of
the quarantine batches to decrease predictability of allocation
patterns when it is enabled.

Diff Detail

Repository
rL LLVM

Event Timeline

cryptoad created this revision.May 1 2019, 9:17 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 1 2019, 9:17 AM
Herald added subscribers: Restricted Project, jfb, delcypher, mgorny. · View Herald Transcript
morehouse added inline comments.May 6 2019, 10:59 AM
lib/scudo/standalone/quarantine.h
138 ↗(On Diff #197568)

Does range-based loop work here?

157 ↗(On Diff #197568)

Since uptr, probably want %zd for all.

181 ↗(On Diff #197568)

What range of MaxCacheSize will Scudo allow? I notice you removed handling for MaxCacheSize == 0 in put below.

188 ↗(On Diff #197568)

I think these can be initLinkerInitialized.

188 ↗(On Diff #197568)

Right now it's a no-op, but I think we should call Cache.initLinkerInitialized here too in case a future change makes it not a no-op.

cryptoad marked 4 inline comments as done.May 6 2019, 11:44 AM
cryptoad added inline comments.
lib/scudo/standalone/quarantine.h
138 ↗(On Diff #197568)

If you could please clarify, I am unsure as to which range you are referring to.

181 ↗(On Diff #197568)

This is due to the fact that the Combined will skip a put and directly deallocate a chunk if:

  • the global quarantine cache size is 0
  • or the size of the chunk is greater than quarantine_max_chunk_size
  • or the size of a chunk is 0 (as requested by user)

This is the current behavior of Scudo with quarantineOrDeallocateChunk, except that since we were using the sanitizer_common Quarantine, the put were doing an extraneous comparison.

As for range of values, they will likely be identical to the ones currently implemented in the non-standalone version.

188 ↗(On Diff #197568)

Thanks, it turned out StaticMutex didn't have an initLinkerInitialized, so I added one.

cryptoad updated this revision to Diff 198319.May 6 2019, 11:47 AM
cryptoad marked an inline comment as done.

Addressing Matt's comments:

  • changing some initializers to initLinkerInitialized. This also required implementing a no-op version of this function for StaticSpinMutex
  • changed a %d to %zd
morehouse accepted this revision.May 6 2019, 11:51 AM
morehouse added inline comments.
lib/scudo/standalone/quarantine.h
138 ↗(On Diff #197568)
for (const QuarantineBatch &Batch : List) {
  ...
}
157 ↗(On Diff #197568)

Also for memory overhead

This revision is now accepted and ready to land.May 6 2019, 11:51 AM
cryptoad marked 6 inline comments as done.May 6 2019, 12:14 PM
cryptoad added inline comments.
lib/scudo/standalone/quarantine.h
138 ↗(On Diff #197568)

It actually does work, thanks! Changed it.

157 ↗(On Diff #197568)

Doh! Sorry I missed it. Also I forgot to clang-format post change.

cryptoad updated this revision to Diff 198323.May 6 2019, 12:15 PM
cryptoad marked 2 inline comments as done.

Addressing Matt's new comments:

  • changing another %d to %zd
  • changing the iterating loop to a range loop
  • clang-format'ing the source
This revision was automatically updated to reflect the committed changes.