Page MenuHomePhabricator

[compiler-rt] Refactor the interception code on windows.
ClosedPublic

Authored by etienneb on Jul 7 2016, 2:27 PM.

Details

Summary

This is a cleanup and refactoring of the interception code on windows

Enhancement:

  • Adding the support for 64-bits code
  • Adding several hooking technique:
    • Detour
    • JumpRedirect
    • HotPatch
    • Trampoline
  • Adding a trampoline memory pool (64-bits) and release the allocated memory in unittests

Cleanup:

  • Adding unittests for 64-bits hooking techniques
  • Enhancing the RoundUpInstruction by sharing common decoder

Diff Detail

Event Timeline

etienneb updated this revision to Diff 63138.Jul 7 2016, 2:27 PM
etienneb retitled this revision from to [compiler-rt] Refactor the interception code on windows..
etienneb updated this object.
etienneb added a reviewer: rnk.
rnk added inline comments.Jul 7 2016, 3:59 PM
lib/interception/interception_win.cc
230–242

static

236

static

243

Doesn't this scan forwards, and aren't we trying to scan backwards?

645

These local variables should probably follow google naming conventions, which is the prevailing local style in the sanitizer libraries.

etienneb updated this revision to Diff 63180.Jul 8 2016, 12:10 AM
etienneb marked 4 inline comments as done.

add 64-bits unittests

added some 64-bits unittests.

lib/interception/interception_win.cc
243

I'll rename the function.
It's scanning a memory region.
The computation for the nop is done at the call size.
This function will have other usage, so let rename it.

645

ok. I'm used to take the llvm coding style.

etienneb updated this revision to Diff 63249.Jul 8 2016, 9:40 AM

more unittests

etienneb updated this object.Jul 8 2016, 9:51 AM
etienneb updated this revision to Diff 63410.Jul 9 2016, 8:31 PM

add more hooking techniques

etienneb updated this revision to Diff 63411.Jul 9 2016, 11:08 PM

cleanup + documentation

etienneb updated this object.Jul 9 2016, 11:10 PM
etienneb updated this object.
etienneb updated this object.
etienneb updated this revision to Diff 63412.Jul 9 2016, 11:14 PM
etienneb updated this object.

upload real patch

etienneb updated this revision to Diff 63415.Jul 10 2016, 12:18 AM

more unittests

etienneb updated this revision to Diff 63416.Jul 10 2016, 12:23 AM

fix typos

etienneb updated this revision to Diff 63451.Jul 10 2016, 7:45 PM

more nits

etienneb updated this revision to Diff 63528.Jul 11 2016, 10:05 AM

decode more instructions

etienneb updated this revision to Diff 63531.Jul 11 2016, 10:12 AM

more nits

etienneb updated this revision to Diff 63533.Jul 11 2016, 10:18 AM

fix dependencies issues

rnk added inline comments.Jul 11 2016, 1:27 PM
lib/interception/interception_win.cc
214

Maybe CHECK(offset >= INT_MIN && offset <= INT_MAX) or an equivalent?

222

Check overflow

238

check overflow

276

We should protect this with a critical section or some other mutex. The sanitizer_common mutex is probably not accessible from here, so I guess we should use a windows critical section.

391

A comment before this case block that these are all control flow instructions that we can't overwrite, and we indicate failure by returning 0.

lib/interception/tests/CMakeLists.txt
124–125 ↗(On Diff #63533)

This file is otherwise unchanged, so I'd revert this whitespace-only change.

lib/interception/tests/interception_win_test.cc
39–40

I don't think these need to be functors, you can just pass function pointers to TestIdentityFunctionPatching and the other guy.

316–318

Do you think this is cleaner without the functor? Do you think we'll need to put other stuff in the overrider class?

I think you can use plain function pointers like this:

typedef void (*Overrider)(uptr, uptr, uptr*);
Overrider o = &OverrideFunctionWithDetour;
FunctionPrefixKind prefix = FunctionPrefixDetour;
TestIdentityFunctionPatching(kIdenti..., prefix, o);
TestIdentityFunctionPatching(kIdenti..., prefix, o);
TestIdentityFunctionPatching(kIdenti..., prefix, o);
etienneb updated this revision to Diff 63599.Jul 11 2016, 3:31 PM
etienneb marked 7 inline comments as done.

address rnk@ comments

lib/interception/interception_win.cc
276

I do not see why.
I was thinking the patching must occur before threads creation (or after a StopTheWorld).

Otherwise, dynamic rewriting the code will also need critical section.

lib/interception/tests/interception_win_test.cc
316–318

Both are fine to me.
I do not think we need to put more into the functor.
Moved to function pointer.

rnk accepted this revision.Jul 11 2016, 3:38 PM
rnk edited edge metadata.

lgtm

lib/interception/interception_win.cc
276

OK, I believe that.

lib/interception/tests/interception_win_test.cc
273

This should be 'override' instead of 'Override', since it's a variable. Also below.

This revision is now accepted and ready to land.Jul 11 2016, 3:38 PM
etienneb updated this revision to Diff 63606.Jul 11 2016, 4:05 PM
etienneb edited edge metadata.

nits

etienneb closed this revision.Jul 11 2016, 4:09 PM