This is an archive of the discontinued LLVM Phabricator instance.

[GWP-ASan] enable/disable and fork support.
ClosedPublic

Authored by eugenis on Jan 23 2020, 2:04 PM.

Details

Summary
  • Implement enable() and disable() in GWP-ASan.
  • Setup atfork handler.
  • Improve test harness sanity and re-enable GWP-ASan in Scudo.

Scudo_standalone disables embedded GWP-ASan as necessary around fork().
Standalone GWP-ASan sets the atfork handler in init() if asked to. This
requires a working malloc(), therefore GWP-ASan initialization in Scudo
is delayed to the post-init callback.

Test harness changes are about setting up a single global instance of
the GWP-ASan allocator so that pthread_atfork() does not create
dangling pointers.

Test case shamelessly stolen from D72470.

Diff Detail

Event Timeline

eugenis created this revision.Jan 23 2020, 2:04 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added subscribers: Restricted Project, jfb, mgorny. · View Herald Transcript

I've merged two earlier changes (gwp-asan re-enablement and atfork/enable/disable support) in one because they are interconnected.

hctim added inline comments.Jan 23 2020, 3:10 PM
compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
116

// AdjustedSampleRatePlusOne is designed to intentionally underflow. This class must be valid when zero-initialised, and we wish to sample as infrequently as possible when this is the case, hence we underflow to UINT32_MAX.

125

Now that GuardedPagePool is zero-initialised, the fast path has an additional branch. Can be fixed via:
return P < GuardedPagePoolEnd && GuardedPagePool <= P;

compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_posix.cpp
95

Why does this need to disable the singleton? Why not just disable this class (which should be equivalent).

compiler-rt/lib/gwp_asan/tests/enable_disable.cpp
46

Nit: prefer c++-style casting

47

What do mean by "initialize" here? GPA should already be inited by the parent.

If you're trying to init the sample counters to some nonzero value here, calling shouldSample() is the only way, not allocate().

compiler-rt/lib/gwp_asan/tests/harness.cpp
8

OnlyOnce?

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

Why a single global instance?

eugenis updated this revision to Diff 240061.Jan 23 2020, 4:58 PM
eugenis marked 6 inline comments as done.

address comments

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

Sure.

compiler-rt/lib/gwp_asan/tests/enable_disable.cpp
47

That's left over from the scudo tests. Removed the initialization and cleaned up synchronization a bit. We can not get rid of the sleep, unfortunately, because the thread needs to act while the other one is blocked inside fork(), and there is no clear way to detect that condition.

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

Yeah, I don't think it is required. We do need SetUp/TearDown logic though to properly set and remove the signal handlers.

hctim accepted this revision.Jan 23 2020, 5:33 PM

LGTM with installAtFork() and cond_wait() comments.

compiler-rt/lib/gwp_asan/tests/enable_disable.cpp
52

I don't see why this value is needed - we don't expect someone else to send the cond_signal for this conditional, right?

This revision is now accepted and ready to land.Jan 23 2020, 5:33 PM

Unit tests: fail. 62144 tests passed, 5 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 32 errors and 19 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 marked an inline comment as done.Jan 24 2020, 1:25 PM
eugenis added inline comments.
compiler-rt/lib/gwp_asan/tests/enable_disable.cpp
52

pthread_cond_wait is allowed spurious wake ups.
Using it without a loop with an external condition is almost always wrong.

eugenis updated this revision to Diff 240281.Jan 24 2020, 1:25 PM

fix lint warnings

Unit tests: pass. 62190 tests passed, 0 failed and 815 were skipped.

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

clang-format: pass.

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.

This revision was automatically updated to reflect the committed changes.