This is an archive of the discontinued LLVM Phabricator instance.

[GWP-ASan] Fix up bad report for in-page underflow w/ UaF
ClosedPublic

Authored by hctim on Dec 12 2022, 3:06 PM.

Details

Summary

Complex scenario, but reports when there's both a use-after-free and
buffer-underflow that is in-page (i.e. doesn't touch the guard page)
ended up generating a pretty bad report:

'Use After Free at 0x7ff392e88fef (18446744073709551615 bytes into a
1-byte allocation at 0x7ff392e88ff0) by thread 3836722 here:'

(note the 2^64-bytes-into-alloc, very cool and good!)

Fix up that case, and add a diagnostic about when you have both a
use-after-free and a buffer-overflow that it's probably a bogus report
(assuming the developer didn't *really* screw up and have a uaf+overflow
bug at the same time).

Diff Detail

Event Timeline

hctim created this revision.Dec 12 2022, 3:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2022, 3:06 PM
Herald added a subscriber: Enna1. · View Herald Transcript
hctim requested review of this revision.Dec 12 2022, 3:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2022, 3:06 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
hctim updated this revision to Diff 482305.Dec 12 2022, 4:21 PM

Make it so overflow+uaf reports uaf first, which is probably the more important bit of information, and makes the reporting more consistent.

hctim updated this revision to Diff 486073.Jan 3 2023, 1:49 PM

Rebased. vitalybuka / eugenis, PTAL

vitalybuka accepted this revision.Jan 4 2023, 11:51 AM
This revision is now accepted and ready to land.Jan 4 2023, 11:51 AM
vitalybuka added inline comments.Jan 4 2023, 12:03 PM
compiler-rt/lib/gwp_asan/crash_handler.cpp
56–58 ↗(On Diff #486073)

not a fan of unnecessary local vars, can be source of bugs
e.g. use after scope

Some more reasoning: https://abseil.io/tips/161

58 ↗(On Diff #486073)

{} use is inconsistent

vitalybuka added inline comments.Jan 4 2023, 12:07 PM
compiler-rt/lib/gwp_asan/crash_handler.cpp
75 ↗(On Diff #486073)

here another one with the same name, reader can be confused which one is used
but this one is hard to remove.

hctim updated this revision to Diff 487876.Jan 10 2023, 10:29 AM
hctim marked 3 inline comments as done.

Touch up a local variable.

compiler-rt/lib/gwp_asan/crash_handler.cpp
56–58 ↗(On Diff #486073)

yeah, especially as it shadows the same name in the scope below. thanks.

This revision was landed with ongoing or failed builds.Jan 10 2023, 10:30 AM
This revision was automatically updated to reflect the committed changes.