Page MenuHomePhabricator

[GWP-ASan] Add aligned allocations.
ClosedPublic

Authored by hctim on Jan 15 2021, 2:39 PM.

Details

Summary

Adds a new allocation API to GWP-ASan that handles size+alignment
restrictions.

Diff Detail

Event Timeline

hctim requested review of this revision.Jan 15 2021, 2:39 PM
hctim created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2021, 2:39 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Is there some test that can be added that would find a regression?

eugenis added inline comments.Jan 15 2021, 2:59 PM
compiler-rt/lib/gwp_asan/optional/backtrace.h
18 ↗(On Diff #317080)

unrelated comment

compiler-rt/lib/gwp_asan/tests/harness.h
38 ↗(On Diff #317080)

extra new line

compiler-rt/lib/scudo/scudo_allocator.cpp
321 ↗(On Diff #317080)

split hooks into a separate commit, or at least mention them in the message

compiler-rt/lib/scudo/standalone/combined.h
485

another unrelated change

hctim edited the summary of this revision. (Show Details)Jan 26 2021, 4:26 PM
hctim marked 4 inline comments as done.Jan 26 2021, 4:37 PM

Is there some test that can be added that would find a regression?

I'm looking into some cmake magic to also run the scudo-standalone tests using GWP_ASAN_OPTIONS=MaxSimultaneousAllocations=10000:SampleRate=1 as well, but it's taking some time... :(.

compiler-rt/lib/scudo/standalone/combined.h
485

added details to the commit message

hctim updated this revision to Diff 319440.Jan 26 2021, 4:45 PM
hctim marked an inline comment as done.

Address reviewer comments.

Change the API to take an alignment, not a 'size' and 'real size' paramater. This way, GWP-ASan can use its own alignment guarantees (page alignment).

Needs a little bit more WIP to get the alignment tighter.

hctim planned changes to this revision.Jan 26 2021, 4:46 PM
hctim retitled this revision from [GWP-ASan] Add 'user requested' size and fix Scudo. to [GWP-ASan] Add aligned allocations..Feb 1 2021, 11:09 AM
hctim edited the summary of this revision. (Show Details)
hctim updated this revision to Diff 320532.Feb 1 2021, 11:09 AM

Removed Scudo bits for now, made this GWP-ASan specific. Changes to scudo will come in a follow-up patch.

hctim added inline comments.Feb 1 2021, 11:12 AM
compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp
163

Note to reviewers: Please check my logic here. This specifically helps with the case where we have a 0x4000 byte slot, and we ask for a 0x1 byte allocation with 0x4000 alignment - this allocation can obviously be resident.

cryptoad added inline comments.Feb 2 2021, 12:40 PM
compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp
37

nit: capital X

199

Should this be 1 or alignof(max_align_t)?

hctim updated this revision to Diff 320909.Feb 2 2021, 2:31 PM
hctim marked 2 inline comments as done.

Addressed Kostya's comments.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 2 2021, 2:45 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
hctim reopened this revision.Feb 2 2021, 2:48 PM

Accidentally committed this in 9dc06762470cb5a6cde8de5833cb75262e1bacb0, and reverted it in 0dcf3324cfb4429e85b54e857f9bb86f423ffc5e. This is still needing review.

hctim updated this revision to Diff 320917.Feb 2 2021, 2:51 PM

Rebase after kerfuffle.

hctim updated this revision to Diff 320933.Feb 2 2021, 3:34 PM

Rebased on top of D95542, which changed the GWP-ASan flag parsing dependencies. This patch does require a small change on the Scudo side, but the actual integration on top of this API (with some new fixes) is at D95884.

hctim added a comment.Feb 4 2021, 9:15 AM

(friendly ping)

cryptoad accepted this revision.Feb 5 2021, 8:31 AM
This revision is now accepted and ready to land.Feb 5 2021, 8:31 AM
eugenis accepted this revision.Feb 5 2021, 3:13 PM

LGTM with a nit

getAlignedPtr is very unclear to me, and the 3rd argument is always const anyway!

compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp
163

this looks correct

230

alignUp(SlotStart, Alignment)

(add a helper function if we don;t have that in gwp-asan yet)

233

alignDown(SlotEnd - Size, Alignment)

hctim updated this revision to Diff 322158.Feb 8 2021, 10:17 AM
hctim marked 3 inline comments as done.

Address Evgenii's comments.

This revision was landed with ongoing or failed builds.Feb 8 2021, 11:23 AM
This revision was automatically updated to reflect the committed changes.