Skip to content

Commit

Permalink
Don't rely on pthread_atfork when possible
Browse files Browse the repository at this point in the history
Summary:
Because of google/sanitizers#1116, any races detected during pthread_atfork deadlock the process.
For forks issued from folly we can use an instrumented fork version to minimize the amount of code running via pthread_atfork.

Reviewed By: yfeldblum

Differential Revision: D23281093

fbshipit-source-id: fd4d3bf06b7992ff314f631ed899854a8b3f6c4b
  • Loading branch information
andriigrynenko authored and dotconnor committed Mar 18, 2021
1 parent 6bce0a3 commit 24e1c24
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 3 deletions.
19 changes: 16 additions & 3 deletions folly/Subprocess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include <folly/Exception.h>
#include <folly/ScopeGuard.h>
#include <folly/String.h>
#include <folly/detail/AtFork.h>
#include <folly/io/Cursor.h>
#include <folly/lang/Assume.h>
#include <folly/portability/Sockets.h>
Expand Down Expand Up @@ -444,9 +445,15 @@ void Subprocess::spawnInternal(
if (options.detach_) {
// If we are detaching we must use fork() instead of vfork() for the first
// fork, since we aren't going to simply call exec() in the child.
pid = fork();
pid = detail::AtFork::forkInstrumented(fork);
} else {
pid = vfork();
if (kIsSanitizeThread) {
// TSAN treats vfork as fork, so use the instrumented version
// instead
pid = detail::AtFork::forkInstrumented(fork);
} else {
pid = vfork();
}
}
#ifdef __linux__
}
Expand All @@ -461,7 +468,13 @@ void Subprocess::spawnInternal(
pid = syscall(SYS_clone, *options.cloneFlags_, 0, nullptr, nullptr);
} else {
#endif
pid = vfork();
if (kIsSanitizeThread) {
// TSAN treats vfork as fork, so use the instrumented version
// instead
pid = detail::AtFork::forkInstrumented(fork);
} else {
pid = vfork();
}
#ifdef __linux__
}
#endif
Expand Down
70 changes: 70 additions & 0 deletions folly/detail/AtFork.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,51 @@ namespace detail {

namespace {

#if !FOLLY_MOBILE && !defined(__APPLE__) && !defined(_MSC_VER)
#define FOLLY_ATFORK_USE_FOLLY_TLS 1
#else
#define FOLLY_ATFORK_USE_FOLLY_TLS 0
#endif

template <bool enabled = FOLLY_ATFORK_USE_FOLLY_TLS>
struct SkipAtForkHandlers;

template <>
struct SkipAtForkHandlers<false> {
static bool available() {
return false;
}

static bool get() {
return false;
}

[[noreturn]] static void set(bool) {
std::terminate();
}
};

#if FOLLY_ATFORK_USE_FOLLY_TLS
template <>
struct SkipAtForkHandlers<true> {
static bool available() {
return true;
}

static bool get() {
return value_;
}

static void set(bool value) {
value_ = value;
}

private:
static FOLLY_TLS bool value_;
};
FOLLY_TLS bool SkipAtForkHandlers<true>::value_{false};
#endif

struct AtForkTask {
void const* handle;
folly::Function<bool()> prepare;
Expand All @@ -45,6 +90,9 @@ class AtForkList {
}

static void prepare() noexcept {
if (SkipAtForkHandlers<>::get()) {
return;
}
instance().tasksLock.lock();
while (true) {
auto& tasks = instance().tasks;
Expand All @@ -64,6 +112,9 @@ class AtForkList {
}

static void parent() noexcept {
if (SkipAtForkHandlers<>::get()) {
return;
}
auto& tasks = instance().tasks;
for (auto& task : tasks) {
task.parent();
Expand All @@ -72,6 +123,9 @@ class AtForkList {
}

static void child() noexcept {
if (SkipAtForkHandlers<>::get()) {
return;
}
// if we fork a multithreaded process
// some of the TSAN mutexes might be locked
// so we just enable ignores for everything
Expand Down Expand Up @@ -138,5 +192,21 @@ void AtFork::unregisterHandler(void const* handle) {
}
}

pid_t AtFork::forkInstrumented(fork_t forkFn) {
if (SkipAtForkHandlers<>::available()) {
AtForkList::prepare();
SkipAtForkHandlers<>::set(true);
auto ret = forkFn();
SkipAtForkHandlers<>::set(false);
if (ret) {
AtForkList::parent();
} else {
AtForkList::child();
}
return ret;
} else {
return forkFn();
}
}
} // namespace detail
} // namespace folly
3 changes: 3 additions & 0 deletions folly/detail/AtFork.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ struct AtFork {
folly::Function<void()> parent,
folly::Function<void()> child);
static void unregisterHandler(void const* handle);

using fork_t = pid_t();
static pid_t forkInstrumented(fork_t forkFn);
};

} // namespace detail
Expand Down

0 comments on commit 24e1c24

Please sign in to comment.