Skip to content

Commit

Permalink
[msan] Make origin tracking fork-safe.
Browse files Browse the repository at this point in the history
Chained origins make plain memory stores async-signal-unsafe.
We already disable it inside signal handlers.
This change grabs all origin-related locks before fork() and releases
them after fork() to avoid a deadlock in the child process.

llvm-svn: 217140
  • Loading branch information
eugenis committed Sep 4, 2014
1 parent fb98b74 commit bb91e02
Show file tree
Hide file tree
Showing 7 changed files with 183 additions and 0 deletions.
8 changes: 8 additions & 0 deletions compiler-rt/lib/msan/msan_chained_origin_depot.cc
Original file line number Diff line number Diff line change
Expand Up @@ -116,4 +116,12 @@ u32 ChainedOriginDepotGet(u32 id, u32 *other) {
return desc.here_id;
}

void ChainedOriginDepotLockAll() {
chainedOriginDepot.LockAll();
}

void ChainedOriginDepotUnlockAll() {
chainedOriginDepot.UnlockAll();
}

} // namespace __msan
3 changes: 3 additions & 0 deletions compiler-rt/lib/msan/msan_chained_origin_depot.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ bool ChainedOriginDepotPut(u32 here_id, u32 prev_id, u32 *new_id);
// Retrieves a stored stack trace by the id.
u32 ChainedOriginDepotGet(u32 id, u32 *other);

void ChainedOriginDepotLockAll();
void ChainedOriginDepotUnlockAll();

} // namespace __msan

#endif // MSAN_CHAINED_ORIGIN_DEPOT_H
19 changes: 19 additions & 0 deletions compiler-rt/lib/msan/msan_interceptors.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,24 @@ INTERCEPTOR(void *, shmat, int shmid, const void *shmaddr, int shmflg) {
return p;
}

static void BeforeFork() {
StackDepotLockAll();
ChainedOriginDepotLockAll();
}

static void AfterFork() {
ChainedOriginDepotUnlockAll();
StackDepotUnlockAll();
}

INTERCEPTOR(int, fork, void) {
ENSURE_MSAN_INITED();
BeforeFork();
int pid = REAL(fork)();
AfterFork();
return pid;
}

struct MSanInterceptorContext {
bool in_interceptor_scope;
};
Expand Down Expand Up @@ -1532,6 +1550,7 @@ void InitializeInterceptors() {
INTERCEPT_FUNCTION(tzset);
INTERCEPT_FUNCTION(__cxa_atexit);
INTERCEPT_FUNCTION(shmat);
INTERCEPT_FUNCTION(fork);

inited = 1;
}
Expand Down
8 changes: 8 additions & 0 deletions compiler-rt/lib/sanitizer_common/sanitizer_stackdepot.cc
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,14 @@ const uptr *StackDepotGet(u32 id, uptr *size) {
return desc.stack;
}

void StackDepotLockAll() {
theDepot.LockAll();
}

void StackDepotUnlockAll() {
theDepot.UnlockAll();
}

bool StackDepotReverseMap::IdDescPair::IdComparator(
const StackDepotReverseMap::IdDescPair &a,
const StackDepotReverseMap::IdDescPair &b) {
Expand Down
3 changes: 3 additions & 0 deletions compiler-rt/lib/sanitizer_common/sanitizer_stackdepot.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ StackDepotHandle StackDepotPut_WithHandle(const uptr *stack, uptr size);
// Retrieves a stored stack trace by the id.
const uptr *StackDepotGet(u32 id, uptr *size);

void StackDepotLockAll();
void StackDepotUnlockAll();

// Instantiating this class creates a snapshot of StackDepot which can be
// efficiently queried with StackDepotGet(). You can use it concurrently with
// StackDepot, but the snapshot is only guaranteed to contain those stack traces
Expand Down
19 changes: 19 additions & 0 deletions compiler-rt/lib/sanitizer_common/sanitizer_stackdepotbase.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ class StackDepotBase {

StackDepotStats *GetStats() { return &stats; }

void LockAll();
void UnlockAll();

private:
static Node *find(Node *s, args_type args, u32 hash);
static Node *lock(atomic_uintptr_t *p);
Expand Down Expand Up @@ -153,5 +156,21 @@ StackDepotBase<Node, kReservedBits, kTabSizeLog>::Get(u32 id) {
return args_type();
}

template <class Node, int kReservedBits, int kTabSizeLog>
void StackDepotBase<Node, kReservedBits, kTabSizeLog>::LockAll() {
for (int i = 0; i < kTabSize; ++i) {
lock(&tab[i]);
}
}

template <class Node, int kReservedBits, int kTabSizeLog>
void StackDepotBase<Node, kReservedBits, kTabSizeLog>::UnlockAll() {
for (int i = 0; i < kTabSize; ++i) {
atomic_uintptr_t *p = &tab[i];
uptr s = atomic_load(p, memory_order_relaxed);
unlock(p, (Node *)(s & ~1UL));
}
}

} // namespace __sanitizer
#endif // SANITIZER_STACKDEPOTBASE_H
123 changes: 123 additions & 0 deletions compiler-rt/test/msan/fork.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
// Test that chained origins are fork-safe.
// Run a number of threads that create new chained origins, then fork
// and verify that origin reads do not deadlock in the child process.

// RUN: %clangxx_msan -std=c++11 -fsanitize-memory-track-origins=2 -g -m64 -O3 %s -o %t
// RUN: MSAN_OPTIONS=store_context_size=1000,origin_history_size=0,origin_history_per_stack_limit=0 %run %t |& FileCheck %s

// Fun fact: if test output is redirected to a file (as opposed to
// being piped directly to FileCheck), we may lose some "done"s due to
// a kernel bug:
// https://lkml.org/lkml/2014/2/17/324


#include <pthread.h>
#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <sys/time.h>
#include <signal.h>
#include <errno.h>
#include <atomic>

#include <sanitizer/msan_interface.h>

std::atomic<bool> done;

void copy_uninit_thread2() {
volatile int x;
volatile int v;
while (true) {
v = x;
x = v;
if (done.load())
return;
}
}

void copy_uninit_thread1(int level) {
if (!level)
copy_uninit_thread2();
else
copy_uninit_thread1(level - 1);
}

void *copy_uninit_thread(void *id) {
copy_uninit_thread1((long)id);
return 0;
}

// Run through stackdepot in the child process.
// If any of the hash table cells are locked, this may deadlock.
void child() {
volatile int x;
volatile int v;
for (int i = 0; i < 10000; ++i) {
v = x;
x = v;
}
write(2, "done\n", 5);
}

void test() {
done.store(false);
const int kThreads = 10;
pthread_t t[kThreads];
for (int i = 0; i < kThreads; ++i)
pthread_create(&t[i], NULL, copy_uninit_thread, (void*)(long)i);
usleep(100000);
pid_t pid = fork();
if (pid) {
// parent
done.store(true);
usleep(1000000);
kill(pid, SIGKILL);
} else {
// child
child();
}
}

int main() {
const int kChildren = 20;
for (int i = 0; i < kChildren; ++i) {
pid_t pid = fork();
if (pid) {
// parent
} else {
test();
exit(0);
}
}

for (int i = 0; i < kChildren; ++i) {
pid_t p;
while ((p = wait(NULL)) == -1) { }
}

return 0;
}

// Expect 20 (== kChildren) "done" messages.
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done
// CHECK: done

0 comments on commit bb91e02

Please sign in to comment.