[Lldb-commits] [lldb] caa7e76 - [lldb] Make ProcessLauncherPosixFork (mostly) async-signal-safe
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Wed Dec 29 01:01:59 PST 2021
Author: Pavel Labath
Date: 2021-12-29T09:45:47+01:00
New Revision: caa7e765e5ae250c67eab3edd7cd324d3634f779
URL: https://github.com/llvm/llvm-project/commit/caa7e765e5ae250c67eab3edd7cd324d3634f779
DIFF: https://github.com/llvm/llvm-project/commit/caa7e765e5ae250c67eab3edd7cd324d3634f779.diff
LOG: [lldb] Make ProcessLauncherPosixFork (mostly) async-signal-safe
Multithreaded applications using fork(2) need to be extra careful about
what they do in the fork child. Without any special precautions (which
only really work if you can fully control all threads) they can only
safely call async-signal-safe functions. This is because the forked
child will contain snapshot of the parents memory at a random moment in
the execution of all of the non-forking threads (this is where the
similarity with signals comes in).
For example, the other threads could have been holding locks that can
now never be released in the child process and any attempt to obtain
them would block. This is what sometimes happen when using tcmalloc --
our fork child ends up hanging in the memory allocation routine. It is
also what happened with our logging code, which is why we added a
pthread_atfork hackaround.
This patch implements a proper fix to the problem, by which is to make
the child code async-signal-safe. The ProcessLaunchInfo structure is
transformed into a simpler ForkLaunchInfo representation, one which can
be read without allocating memory and invoking complex library
functions.
Strictly speaking this implementation is not async-signal-safe, as it
still invokes library functions outside of the posix-blessed set of
entry points. Strictly adhering to the spec would mean reimplementing a
lot of the functionality in pure C, so instead I rely on the fact that
any reasonable implementation of some functions (e.g.,
basic_string::c_str()) will not start allocating memory or doing other
unsafe things.
The new child code does not call into our logging infrastructure, which
enables us to remove the pthread_atfork call from there.
Differential Revision: https://reviews.llvm.org/D116165
Added:
Modified:
lldb/include/lldb/Utility/Log.h
lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
lldb/source/Utility/Log.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Utility/Log.h b/lldb/include/lldb/Utility/Log.h
index 01edec0445650..2684783939bd8 100644
--- a/lldb/include/lldb/Utility/Log.h
+++ b/lldb/include/lldb/Utility/Log.h
@@ -211,8 +211,6 @@ class Log final {
static uint32_t GetFlags(llvm::raw_ostream &stream, const ChannelMap::value_type &entry,
llvm::ArrayRef<const char *> categories);
- static void DisableLoggingChild();
-
Log(const Log &) = delete;
void operator=(const Log &) = delete;
};
diff --git a/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp b/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
index 2f08b9fa8857a..635dbb14a0271 100644
--- a/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ b/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -38,43 +38,40 @@
using namespace lldb;
using namespace lldb_private;
-static void FixupEnvironment(Environment &env) {
-#ifdef __ANDROID__
- // If there is no PATH variable specified inside the environment then set the
- // path to /system/bin. It is required because the default path used by
- // execve() is wrong on android.
- env.try_emplace("PATH", "/system/bin");
-#endif
+// Begin code running in the child process
+// NB: This code needs to be async-signal safe, since we're invoking fork from
+// multithreaded contexts.
+
+static void write_string(int error_fd, const char *str) {
+ int r = write(error_fd, str, strlen(str));
+ (void)r;
}
[[noreturn]] static void ExitWithError(int error_fd,
const char *operation) {
int err = errno;
- llvm::raw_fd_ostream os(error_fd, true);
- os << operation << " failed: " << llvm::sys::StrError(err);
- os.flush();
+ write_string(error_fd, operation);
+ write_string(error_fd, " failed: ");
+ // strerror is not guaranteed to be async-signal safe, but it usually is.
+ write_string(error_fd, strerror(err));
_exit(1);
}
-static void DisableASLRIfRequested(int error_fd, const ProcessLaunchInfo &info) {
+static void DisableASLR(int error_fd) {
#if defined(__linux__)
- if (info.GetFlags().Test(lldb::eLaunchFlagDisableASLR)) {
- const unsigned long personality_get_current = 0xffffffff;
- int value = personality(personality_get_current);
- if (value == -1)
- ExitWithError(error_fd, "personality get");
-
- value = personality(ADDR_NO_RANDOMIZE | value);
- if (value == -1)
- ExitWithError(error_fd, "personality set");
- }
+ const unsigned long personality_get_current = 0xffffffff;
+ int value = personality(personality_get_current);
+ if (value == -1)
+ ExitWithError(error_fd, "personality get");
+
+ value = personality(ADDR_NO_RANDOMIZE | value);
+ if (value == -1)
+ ExitWithError(error_fd, "personality set");
#endif
}
-static void DupDescriptor(int error_fd, const FileSpec &file_spec, int fd,
- int flags) {
- int target_fd = llvm::sys::RetryAfterSignal(-1, ::open,
- file_spec.GetCString(), flags, 0666);
+static void DupDescriptor(int error_fd, const char *file, int fd, int flags) {
+ int target_fd = llvm::sys::RetryAfterSignal(-1, ::open, file, flags, 0666);
if (target_fd == -1)
ExitWithError(error_fd, "DupDescriptor-open");
@@ -88,44 +85,67 @@ static void DupDescriptor(int error_fd, const FileSpec &file_spec, int fd,
::close(target_fd);
}
-[[noreturn]] static void ChildFunc(int error_fd,
- const ProcessLaunchInfo &info) {
- if (info.GetFlags().Test(eLaunchFlagLaunchInSeparateProcessGroup)) {
+namespace {
+struct ForkFileAction {
+ ForkFileAction(const FileAction &act);
+
+ FileAction::Action action;
+ int fd;
+ std::string path;
+ int arg;
+};
+
+struct ForkLaunchInfo {
+ ForkLaunchInfo(const ProcessLaunchInfo &info);
+
+ bool separate_process_group;
+ bool debug;
+ bool disable_aslr;
+ std::string wd;
+ const char **argv;
+ Environment::Envp envp;
+ std::vector<ForkFileAction> actions;
+
+ bool has_action(int fd) const {
+ for (const ForkFileAction &action : actions) {
+ if (action.fd == fd)
+ return true;
+ }
+ return false;
+ }
+};
+} // namespace
+
+[[noreturn]] static void ChildFunc(int error_fd, const ForkLaunchInfo &info) {
+ if (info.separate_process_group) {
if (setpgid(0, 0) != 0)
ExitWithError(error_fd, "setpgid");
}
- for (size_t i = 0; i < info.GetNumFileActions(); ++i) {
- const FileAction &action = *info.GetFileActionAtIndex(i);
- switch (action.GetAction()) {
+ for (const ForkFileAction &action : info.actions) {
+ switch (action.action) {
case FileAction::eFileActionClose:
- if (close(action.GetFD()) != 0)
+ if (close(action.fd) != 0)
ExitWithError(error_fd, "close");
break;
case FileAction::eFileActionDuplicate:
- if (dup2(action.GetFD(), action.GetActionArgument()) == -1)
+ if (dup2(action.fd, action.arg) == -1)
ExitWithError(error_fd, "dup2");
break;
case FileAction::eFileActionOpen:
- DupDescriptor(error_fd, action.GetFileSpec(), action.GetFD(),
- action.GetActionArgument());
+ DupDescriptor(error_fd, action.path.c_str(), action.fd, action.arg);
break;
case FileAction::eFileActionNone:
break;
}
}
- const char **argv = info.GetArguments().GetConstArgumentVector();
-
// Change working directory
- if (info.GetWorkingDirectory() &&
- 0 != ::chdir(info.GetWorkingDirectory().GetCString()))
+ if (!info.wd.empty() && 0 != ::chdir(info.wd.c_str()))
ExitWithError(error_fd, "chdir");
- DisableASLRIfRequested(error_fd, info);
- Environment env = info.GetEnvironment();
- FixupEnvironment(env);
- Environment::Envp envp = env.getEnvp();
+ if (info.disable_aslr)
+ DisableASLR(error_fd);
// Clear the signal mask to prevent the child from being affected by any
// masking done by the parent.
@@ -134,7 +154,7 @@ static void DupDescriptor(int error_fd, const FileSpec &file_spec, int fd,
pthread_sigmask(SIG_SETMASK, &set, nullptr) != 0)
ExitWithError(error_fd, "pthread_sigmask");
- if (info.GetFlags().Test(eLaunchFlagDebug)) {
+ if (info.debug) {
// Do not inherit setgid powers.
if (setgid(getgid()) != 0)
ExitWithError(error_fd, "setgid");
@@ -143,6 +163,8 @@ static void DupDescriptor(int error_fd, const FileSpec &file_spec, int fd,
// Close everything besides stdin, stdout, and stderr that has no file
// action to avoid leaking. Only do this when debugging, as elsewhere we
// actually rely on passing open descriptors to child processes.
+ // NB: This code is not async-signal safe, but we currently do not launch
+ // processes for debugging from within multithreaded contexts.
const llvm::StringRef proc_fd_path = "/proc/self/fd";
std::error_code ec;
@@ -157,7 +179,7 @@ static void DupDescriptor(int error_fd, const FileSpec &file_spec, int fd,
// Don't close first three entries since they are stdin, stdout and
// stderr.
- if (fd > 2 && !info.GetFileActionForFD(fd) && fd != error_fd)
+ if (fd > 2 && !info.has_action(fd) && fd != error_fd)
files_to_close.push_back(fd);
}
for (int file_to_close : files_to_close)
@@ -166,7 +188,7 @@ static void DupDescriptor(int error_fd, const FileSpec &file_spec, int fd,
// Since /proc/self/fd didn't work, trying the slow way instead.
int max_fd = sysconf(_SC_OPEN_MAX);
for (int fd = 3; fd < max_fd; ++fd)
- if (!info.GetFileActionForFD(fd) && fd != error_fd)
+ if (!info.has_action(fd) && fd != error_fd)
close(fd);
}
@@ -176,7 +198,7 @@ static void DupDescriptor(int error_fd, const FileSpec &file_spec, int fd,
}
// Execute. We should never return...
- execve(argv[0], const_cast<char *const *>(argv), envp);
+ execve(info.argv[0], const_cast<char *const *>(info.argv), info.envp);
#if defined(__linux__)
if (errno == ETXTBSY) {
@@ -189,7 +211,7 @@ static void DupDescriptor(int error_fd, const FileSpec &file_spec, int fd,
// Since this state should clear up quickly, wait a while and then give it
// one more go.
usleep(50000);
- execve(argv[0], const_cast<char *const *>(argv), envp);
+ execve(info.argv[0], const_cast<char *const *>(info.argv), info.envp);
}
#endif
@@ -198,12 +220,43 @@ static void DupDescriptor(int error_fd, const FileSpec &file_spec, int fd,
ExitWithError(error_fd, "execve");
}
+// End of code running in the child process.
+
+ForkFileAction::ForkFileAction(const FileAction &act)
+ : action(act.GetAction()), fd(act.GetFD()), path(act.GetPath().str()),
+ arg(act.GetActionArgument()) {}
+
+static std::vector<ForkFileAction>
+MakeForkActions(const ProcessLaunchInfo &info) {
+ std::vector<ForkFileAction> result;
+ for (size_t i = 0; i < info.GetNumFileActions(); ++i)
+ result.emplace_back(*info.GetFileActionAtIndex(i));
+ return result;
+}
+
+static Environment::Envp FixupEnvironment(Environment env) {
+#ifdef __ANDROID__
+ // If there is no PATH variable specified inside the environment then set the
+ // path to /system/bin. It is required because the default path used by
+ // execve() is wrong on android.
+ env.try_emplace("PATH", "/system/bin");
+#endif
+ return env.getEnvp();
+}
+
+ForkLaunchInfo::ForkLaunchInfo(const ProcessLaunchInfo &info)
+ : separate_process_group(
+ info.GetFlags().Test(eLaunchFlagLaunchInSeparateProcessGroup)),
+ debug(info.GetFlags().Test(eLaunchFlagDebug)),
+ disable_aslr(info.GetFlags().Test(eLaunchFlagDisableASLR)),
+ wd(info.GetWorkingDirectory().GetPath()),
+ argv(info.GetArguments().GetConstArgumentVector()),
+ envp(FixupEnvironment(info.GetEnvironment())),
+ actions(MakeForkActions(info)) {}
+
HostProcess
ProcessLauncherPosixFork::LaunchProcess(const ProcessLaunchInfo &launch_info,
Status &error) {
- char exe_path[PATH_MAX];
- launch_info.GetExecutableFile().GetPath(exe_path, sizeof(exe_path));
-
// A pipe used by the child process to report errors.
PipePosix pipe;
const bool child_processes_inherit = false;
@@ -211,6 +264,8 @@ ProcessLauncherPosixFork::LaunchProcess(const ProcessLaunchInfo &launch_info,
if (error.Fail())
return HostProcess();
+ const ForkLaunchInfo fork_launch_info(launch_info);
+
::pid_t pid = ::fork();
if (pid == -1) {
// Fork failed
@@ -221,7 +276,7 @@ ProcessLauncherPosixFork::LaunchProcess(const ProcessLaunchInfo &launch_info,
if (pid == 0) {
// child process
pipe.CloseReadFileDescriptor();
- ChildFunc(pipe.ReleaseWriteFileDescriptor(), launch_info);
+ ChildFunc(pipe.ReleaseWriteFileDescriptor(), fork_launch_info);
}
// parent process
diff --git a/lldb/source/Utility/Log.cpp b/lldb/source/Utility/Log.cpp
index ff654ec93e784..26070d0740b19 100644
--- a/lldb/source/Utility/Log.cpp
+++ b/lldb/source/Utility/Log.cpp
@@ -30,7 +30,6 @@
#include <process.h>
#else
#include <unistd.h>
-#include <pthread.h>
#endif
using namespace lldb_private;
@@ -180,9 +179,6 @@ void Log::Warning(const char *format, ...) {
}
void Log::Initialize() {
-#ifdef LLVM_ON_UNIX
- pthread_atfork(nullptr, nullptr, &Log::DisableLoggingChild);
-#endif
InitializeLldbChannel();
}
@@ -346,11 +342,3 @@ void Log::Format(llvm::StringRef file, llvm::StringRef function,
message << payload << "\n";
WriteMessage(message.str());
}
-
-void Log::DisableLoggingChild() {
- // Disable logging by clearing out the atomic variable after forking -- if we
- // forked while another thread held the channel mutex, we would deadlock when
- // trying to write to the log.
- for (auto &c: *g_channel_map)
- c.second.m_channel.log_ptr.store(nullptr, std::memory_order_relaxed);
-}
More information about the lldb-commits
mailing list