Page MenuHomePhabricator

[esan|wset] Intercept and chain signal handlers
ClosedPublic

Authored by bruening on May 24 2016, 10:19 AM.

Details

Summary

In preparation for fault-based shadow memory iteration, we add support for
our own signal handler by adding app signal handler interception as well as
chaining for SIGSEGV. This is done in a simple manner: we do not honor the
app's alternate stack nor any sigaction flags for SIGSEGV.

Adds a new test of transparency in app signal handling.

Diff Detail

Repository
rL LLVM

Event Timeline

bruening updated this revision to Diff 58262.May 24 2016, 10:19 AM
bruening retitled this revision from to [esan|wset] Intercept and chain signal handlers.
bruening updated this object.
bruening added a reviewer: aizatsky.
bruening added subscribers: llvm-commits, eugenis, kcc and 2 others.
aizatsky added inline comments.May 24 2016, 2:28 PM
lib/esan/working_set_posix.cpp
38 ↗(On Diff #58262)

s/skip/skip real/

53 ↗(On Diff #58262)

ditto

99 ↗(On Diff #58262)

Could a signal happen before this? AppSigAct would be uninitialized in process* functions.

test/esan/TestCases/workingset-fault.cpp
1 ↗(On Diff #58262)

This test as it is doesn't test faults, but tests signal handling propagation, right? Maybe rename it?

(Optionally include "posix" in the name?)

filcab added a subscriber: filcab.May 25 2016, 7:24 AM
filcab added inline comments.
lib/esan/esan.h
43 ↗(On Diff #58262)

"Platform dependent"?

lib/esan/working_set.h
28 ↗(On Diff #58262)

Minor nit: dependent? They're not changing the platform.

lib/esan/working_set_posix.cpp
67 ↗(On Diff #58262)

handleSEGV for now. Possibly handleSignal later.
This will be called for any SEGV, and ShadowFault isn't descriptive.

84 ↗(On Diff #58262)

registerSEGVHandler

test/esan/TestCases/workingset-fault.cpp
32 ↗(On Diff #58262)

Would kill(SIGSEGV, getpid()) work?

bruening marked 3 inline comments as done.May 25 2016, 10:04 AM
bruening added inline comments.
lib/esan/working_set_posix.cpp
67 ↗(On Diff #58262)

The purpose of this routine is to handle shadow page write faults.

84 ↗(On Diff #58262)

This routine will have the same name on every platform. Using a UNIX-specific name is not ideal: "fault" will translate to Windows.

99 ↗(On Diff #58262)

I think what you mean is could a call to signal() or sigaction() happen before this? Yes, that is a general issue with all interceptors, and it will be fixed in a separate CL that ensures our whole runtime lib is initialized at the first interceptor that is encountered.

test/esan/TestCases/workingset-fault.cpp
1 ↗(On Diff #58262)

It's both, testing propagation of faults (SIGSEGV) as no other signals need propagation. Sure, posix sounds good.

32 ↗(On Diff #58262)

If POSIX guarantees synchronous delivery before kill returns (and platforms implement that), which I believe is true for a single-threaded app, yes, but a true fault just feels like a more authentic test.

bruening updated this revision to Diff 58450.May 25 2016, 10:06 AM
bruening marked 3 inline comments as done.

Address various reviewer comments.

filcab added inline comments.May 25 2016, 10:22 AM
lib/esan/working_set_posix.cpp
68 ↗(On Diff #58450)

But it handles any and all SEGV, right? Not just in the shadow memory.
A "shadow fault" doesn't really mean much by itself either (unless you're familiar with this code, of course).
Maybe we can agree on handleMemoryFault? I'm ok with not adding the POSIX-related names, since it's generic. But I'd rather not mention Shadow like that in a function that will be called for things outside the shadow memory.

85 ↗(On Diff #58450)

Fair enough. registerMemoryFaultHandler?

test/esan/TestCases/workingset-fault.cpp
32 ↗(On Diff #58262)

Fair enough. No need casting argc twice, btw.
I cringe looking at that line, but couldn't get you an easier way to trigger the fault off the top of my head.

aizatsky accepted this revision.May 25 2016, 12:56 PM
aizatsky edited edge metadata.

LG
Please make sure Filipe is happy as well :)

This revision is now accepted and ready to land.May 25 2016, 12:56 PM
bruening marked an inline comment as done.May 25 2016, 7:03 PM
bruening added inline comments.
lib/esan/working_set_posix.cpp
68 ↗(On Diff #58450)

But it handles any and all SEGV, right? Not just in the shadow memory.

It doesn't take its own action on non-shadow faults: it passes them through to the app. Thus, a non-shadow fault remains un-handled: we let the app handle it. It really depends on how you think of "handle".

A "shadow fault" doesn't really mean much by itself either (unless you're familiar with this code, of course).

I'll assume that what you mean is "I myself have never seen this phrase before". That does not make it a poor term. It is descriptive and self-explanatory in the context of a shadow memory tool.

I am ok changing these names, as it is true that other faults will pass through these routines, but the original names nicely captured what the code is trying to accomplish: to take action on shadow memory faults. We would really prefer to ignore all other faults, but the signal mechanism forces us to chain a general handler with the app's and to pass through the non-shadow faults.

test/esan/TestCases/workingset-fault.cpp
32 ↗(On Diff #58262)

Two casts are needed to avoid a warning about casting a non-pointer-sized integer to a pointer.

bruening updated this revision to Diff 58556.May 25 2016, 7:04 PM
bruening edited edge metadata.

Rename fault handling routines.

This revision was automatically updated to reflect the committed changes.