This is an archive of the discontinued LLVM Phabricator instance.

[GWP-ASan] Crash Handler API.
ClosedPublic

Authored by hctim on Jan 28 2020, 7:30 AM.

Details

Summary

Forewarning: This patch looks big in #LOC changed. I promise it's not that bad, it just moves a lot of content from one file to another. I've gone ahead and left inline comments on Phabricator for sections where this has happened.

This patch:

  1. Introduces the crash handler API (crash_handler_api.h).
  2. Moves information required for out-of-process crash handling into an AllocatorState. This is a trivially-copied POD struct that designed to be recovered from a deceased process, and used by the crash handler to create a GWP-ASan report (along with the other trivially-copied Metadata struct).
  3. Implements the crash handler API using the AllocatorState and Metadata.
  4. Adds tests for the crash handler.
  5. Reimplements the (now optionally linked by the supporting allocator) in-process crash handler (i.e. the segv handler) using the new crash handler API.
  6. Minor updates Scudo & Scudo Standalone to fix compatibility.
  7. Changed capitalisation of errors (e.g. /s/Use after free/Use After Free).

Diff Detail

Event Timeline

hctim created this revision.Jan 28 2020, 7:30 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, Restricted Project, cryptoad, mgorny. · View Herald Transcript
hctim marked 33 inline comments as done.Jan 28 2020, 7:45 AM
hctim added inline comments.
compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp
1

Most of the changes in this file are /s/MyMember/State.MyMember

117–118

No Printf maintained internally any more, so we assert and trap instead.

200

Used to be a member function (GPA::getPageAddr()), but shouldn't be.

307–311

Slight functional change, addrToMetadata used to be implemented differently.

395

Start of implementation of new crash handler functions. All very simple implementation (just find the metadata, and then return data).

413

This code was moved (with some modifications) to crash_handler_posix.cpp.

516

Code here (and below) moved to crash_handler_posix.cpp.

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
22

This is moved just down below in this file.

78

These functions and members are pulled out of GuardedPoolAllocator.

149

Moved into AllocatorState.

153

Removed in favour of new crash handler API.

194

Installation of segv is now in optional/crash_handler.h.

202

Functions moved into AllocatorState.

213

Functions moved into AllocatorState.

235

Deprecated in favour of new crash handler.

247

Members (and below) moved into AllocatorState.

274

We now don't store a Printf or PrintBacktrace function, as these are the crash handler's responsibility (and GPA no longer is responsible for handling crashes).

compiler-rt/lib/gwp_asan/optional/crash_handler.h
18

Moved from options.h. Now Printf_t and PrintBacktrace_t are correctly only used by the inbuilt crash handler, and not managed by GPA.

63

This is now the entry point to install the internal (segv-based) crash handler, and the allocator is responsible for inspecting Options.installSignalHandlers and call this function if true.

The allocator is responsible for providing us with the proper printing/backtrace functions, but we also provide multiple implementations that can be used.

compiler-rt/lib/gwp_asan/optional/crash_handler_posix.cpp
35

Moved from guarded_pool_allocator.cpp.

61

Moved from guarded_pool_allocator.cpp.

69

Moved from guarded_pool_allocator.cpp (with minor modifications to use new API instead of old).

117

Moved from guarded_pool_allocator.cpp (with minor modifications to use new API instead of old).

136

Moved from guarded_pool_allocator.cpp (with minor modifications to use new API instead of old).

158

Moved from guarded_pool_allocator.cpp (with minor modifications to use new API instead of old).

compiler-rt/lib/gwp_asan/options.h
18

Moved to crash_handler_api.h.

hctim added inline comments.Jan 28 2020, 7:45 AM
compiler-rt/lib/gwp_asan/options.h
53

Moved to crash_handler_api.h.

compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_posix.cpp
39–40

No more Printf in GPA, so assert and trap instread.

94

Moved to crash_handler_posix.cpp.

125

Moved to crash_handler_posix.cpp.

compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp
2

New tests for the crash handler API.

compiler-rt/lib/scudo/standalone/CMakeLists.txt
116

Note for @cryptoad: This is the libc backtrace and the GWP-ASan internal (segv) crash handler, no dependencies on compiler-rt here :)

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

Backtrace function comes from libc. Not sure if this WAI on Fuchsia, but we don't support GWP-ASan there so /shrug/.

hctim edited the summary of this revision. (Show Details)Jan 28 2020, 7:46 AM
hctim edited reviewers, added: cryptoad, eugenis; removed: jfb.
hctim added subscribers: jfb, pcc.

Unit tests: pass. 62268 tests passed, 0 failed and 827 were skipped.

clang-tidy: fail. clang-tidy found 21 errors and 58 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

eugenis added inline comments.Jan 28 2020, 6:07 PM
compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp
97–98

If you move backtrace collection (but maybe not compression) to the caller, you can get rid of the ugly passing-by-reference of RecursiveGuard.

117–118

why do you need trap after assert?

Factor out CHECK() implemented (on Android) as set_abort_message() + abort?

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
169

Tries to stop. Not very hard.
If we really wanted to, we could make the mutex recursive.

261

internally

compiler-rt/lib/gwp_asan/options.inc
33

Would it be better to move this option to scudo then if it is unused?

compiler-rt/lib/gwp_asan/tests/CMakeLists.txt
9

Don't understand the comment. Does this optimization break the code? Why? Where did -O2 go?

compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp
2

This test makes my head hurt :(
I don't see why it needs to actually send and receive signals to test this API.
Also, the fuzzy match logic seems to be testing an implementation of backtrace. This also seems irrelevant. I'd simply supply a mock backtrace callback.

24

or can just do attribute((optnone))

217

why not IntPtr = &Intptr ?

compiler-rt/lib/scudo/standalone/CMakeLists.txt
25

this can affect scudo performance.

hctim marked 10 inline comments as done.Jan 29 2020, 4:36 PM
hctim added inline comments.
compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp
97–98

Refactored a little here.

117–118

In case of -DNDEBUG.

Done.

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
169

We need something to set RecursiveGuard = true in the in-process crash handler. The tryLock is mostly there as a best-effort as we might take some time uncompressing traces/getting the access backtrace, and it reduces the chance of our metadata getting corrupted.

Not sure what we could do to stop harder here :(

compiler-rt/lib/gwp_asan/options.inc
33

My assumption is that some new consumer may chose to use the sanitizer_common flag parser (which we include optionally under optional/options_parser.h). In that case, it's nice to have this flag be user-controlled. I'm not super attached to it being here though, LMK.

compiler-rt/lib/gwp_asan/tests/CMakeLists.txt
9

It ensured that call -> jmp optimisation didn't take place, but optnone is a much better sol'n.

compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp
2

Let me refactor this to not go through the actual allocator + crashing stuff. I refactored this test from some working branch that had wildly different assumptions.

217

Sorry, I don't follow...

compiler-rt/lib/scudo/standalone/CMakeLists.txt
25
hctim updated this revision to Diff 241318.Jan 29 2020, 4:36 PM
hctim marked 3 inline comments as done.
  • Remove bad assert from debug builds.
  • Comments from @eugenis.
eugenis added inline comments.Jan 29 2020, 5:06 PM
compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp
217

It looks like the loop above finds any address that GWP-ASan would not recognize in pointerIsMine.
&IntPtr is such an address.

Unit tests: pass. 62268 tests passed, 0 failed and 827 were skipped.

clang-tidy: fail. clang-tidy found 23 errors and 62 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

hctim updated this revision to Diff 241746.Jan 31 2020, 8:59 AM
  • - Updated crash_handler test, fixed up some const-correctness issues.
hctim marked 6 inline comments as done.Jan 31 2020, 9:00 AM
hctim added inline comments.
compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp
2

Refactored.

217

Done.

Unit tests: fail. 62267 tests passed, 1 failed and 827 were skipped.

failed: libc++.std/thread/futures/futures_async/async.pass.cpp

clang-tidy: fail. clang-tidy found 23 errors and 51 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

hctim updated this revision to Diff 241826.Jan 31 2020, 2:54 PM
hctim marked 2 inline comments as done.
Moved functionality out of GPA into common.

Functionality that is common between the GPA and the crash handler
should be factored out. This allows us to link *just* the crash handler
portion of things in a supporting crash handler, rather than all the
rest of the GPA stuff.

Unit tests: pass. 62268 tests passed, 0 failed and 827 were skipped.

clang-tidy: fail. clang-tidy found 28 errors and 52 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

eugenis added inline comments.Jan 31 2020, 5:22 PM
compiler-rt/lib/gwp_asan/crash_handler.cpp
79 ↗(On Diff #241826)

Why not return the metadata pointer and null if it could not be found, and then pass it to all other query functions?

compiler-rt/lib/gwp_asan/optional/segv_handler_posix.cpp
53 ↗(On Diff #241826)

We should not kill the process if it had SIGSEGV ignored, and we do not recognize the crash.

hctim marked 4 inline comments as done.Jan 31 2020, 6:43 PM
hctim added inline comments.
compiler-rt/lib/gwp_asan/crash_handler.cpp
79 ↗(On Diff #241826)

The crash handler isn't responsible for finding the metadata, as the original process might be dead. We actually need the metadata here to check that the allocation provided has actually been allocated once upon a time.

compiler-rt/lib/gwp_asan/optional/segv_handler_posix.cpp
53 ↗(On Diff #241826)

Done.

hctim updated this revision to Diff 241859.Jan 31 2020, 6:44 PM
hctim marked 2 inline comments as done.
  • Crash on SEGV when previous handler ignored iff we are responsible.

Unit tests: pass. 62268 tests passed, 0 failed and 827 were skipped.

clang-tidy: fail. clang-tidy found 28 errors and 52 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

eugenis added inline comments.Feb 3 2020, 4:18 PM
compiler-rt/lib/gwp_asan/crash_handler.cpp
79 ↗(On Diff #241826)

I actually meant instead of

if (Meta->Addr == 0)
    return false;

do

if (Meta->Addr != 0)
  return Meta;

And then pass that pointer to the rest of the API, so they do not need to repeat the look up.

compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp
197–201

if init() does not set the signal handlers, maybe uninit should not remove them.
Also, weak declarations are not very portable.

249

Touch memory in a guard page instead.
This will produce the same type of failure the fault handler is catching, which could be different from raise(SIGSEGV) (which is also posix-only).

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
12

clang-format plz

compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp
159

It's usually better to be a bit more explicit in tests and setup the program state in each test case. In this case, I'd need to look ~2 pages up to understand what 0x7001 refers to. The test class could provide metadata setup helpers, and test case could look something like

metadata(0, 0x7000, 0x20, /*allocated*/false);
State.FailureAddress = 0x7001;
State.FailureType = Error::INVALID_FREE;
EXPECT_TRUE(__gwp_asan_error_is_mine(&State));

hctim marked 9 inline comments as done.Feb 5 2020, 8:14 AM
hctim added inline comments.
compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp
197–201

Removed it, and updated the tests elsewhere.

compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp
159

Great idea, thanks!

compiler-rt/lib/scudo/standalone/CMakeLists.txt
25

Flagged it behind COMPILER_RT_HAS_GWP_ASAN - but still worth assessing for @cryptoad

hctim updated this revision to Diff 242632.Feb 5 2020, 8:14 AM
hctim marked 2 inline comments as done.
  • Addressed eugenis@ comments.

Unit tests: pass. 62268 tests passed, 0 failed and 827 were skipped.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

eugenis accepted this revision.Feb 5 2020, 11:01 AM

LGTM with a few nits

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

Could be simply the first guard page at *GuardPagePool. Does not really matter.

compiler-rt/lib/gwp_asan/tests/harness.h
19

this does not exist

93

Not a big deal, but it makes sense to do these things in the opposite order.

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

wrong path

This revision is now accepted and ready to land.Feb 5 2020, 11:01 AM
hctim updated this revision to Diff 242765.Feb 5 2020, 3:24 PM
hctim marked 12 inline comments as done.
  • Update from eugenis comments.
hctim added inline comments.Feb 5 2020, 3:29 PM
compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp
248

Sure. Done.

compiler-rt/lib/gwp_asan/tests/harness.h
19

it does, just under the GWP-ASan root's gwp_asan/optional dir, not gwp_asan/tests/optional/

93

Done (and in Scudo).

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

why does my editor hate me so

hctim updated this revision to Diff 242769.Feb 5 2020, 3:31 PM

Rebase onto master in preparation for submit.

This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 62268 tests passed, 0 failed and 827 were skipped.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: pass. 62519 tests passed, 0 failed and 844 were skipped.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.