From dccde943690b2a4af8e1730c6da45f91a01ab318 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 18 Oct 2024 20:24:54 -0700 Subject: [PATCH] daemon: stop eating SIGINTs Daemon client handler processes are forked off of the main nix process and thus will not have a signal handler thread anymore. This leads to a high likelihood of bustage, since the Worker infrastructure expects the interrupt infrastructure to actually, you know, work, to be able to get interrupted. The expected behaviour after fork is either: - Start a signal handler thread if you expect to do complicated things that need ReceiveInterrupts. - Call restoreProcessContext and don't handle signals specially otherwise. Change-Id: I73d36b5bbf96dddd21d5e1c3bd0484d715c00e8b --- src/libstore/build/child.cc | 2 +- src/libstore/build/child.hh | 8 +++- src/libstore/build/hook-instance.cc | 2 +- src/libstore/build/local-derivation-goal.cc | 2 +- src/libutil/current-process.hh | 5 ++- src/libutil/signals.cc | 9 ++++- src/libutil/signals.hh | 44 +++++++++++++++++++-- src/nix/daemon.cc | 4 ++ 8 files changed, 63 insertions(+), 13 deletions(-) diff --git a/src/libstore/build/child.cc b/src/libstore/build/child.cc index a82a5eec9..3b675d8b4 100644 --- a/src/libstore/build/child.cc +++ b/src/libstore/build/child.cc @@ -3,7 +3,7 @@ namespace nix { -void commonChildInit() +void commonExecveingChildInit() { logger = makeSimpleLogger(); diff --git a/src/libstore/build/child.hh b/src/libstore/build/child.hh index 3dfc552b9..3464865e8 100644 --- a/src/libstore/build/child.hh +++ b/src/libstore/build/child.hh @@ -4,8 +4,12 @@ namespace nix { /** - * Common initialisation performed in child processes. + * Common initialisation performed in child processes that are just going to + * execve. + * + * These processes may not use ReceiveInterrupts as they do not have an + * interrupt receiving thread. */ -void commonChildInit(); +void commonExecveingChildInit(); } diff --git a/src/libstore/build/hook-instance.cc b/src/libstore/build/hook-instance.cc index 521f34917..6b3035b99 100644 --- a/src/libstore/build/hook-instance.cc +++ b/src/libstore/build/hook-instance.cc @@ -45,7 +45,7 @@ HookInstance::HookInstance() if (dup2(fromHook_.writeSide.get(), STDERR_FILENO) == -1) throw SysError("cannot pipe standard error into log file"); - commonChildInit(); + commonExecveingChildInit(); if (chdir("/") == -1) throw SysError("changing into /"); diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index c8c68f99f..6f5657617 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -1364,7 +1364,7 @@ void LocalDerivationGoal::runChild() try { /* child */ - commonChildInit(); + commonExecveingChildInit(); setupSyscallFilter(); diff --git a/src/libutil/current-process.hh b/src/libutil/current-process.hh index 8d5a2791d..245946368 100644 --- a/src/libutil/current-process.hh +++ b/src/libutil/current-process.hh @@ -23,8 +23,9 @@ void setStackSize(rlim_t stackSize); /** * Restore the original inherited Unix process context (such as signal - * masks, stack size). - + * masks, stack size). This should generally be called after fork for a process + * intending to simply execve. + * * See startSignalHandlerThread(), saveSignalMask(). */ void restoreProcessContext(bool restoreMounts = true); diff --git a/src/libutil/signals.cc b/src/libutil/signals.cc index dac2964ae..6d5201a78 100644 --- a/src/libutil/signals.cc +++ b/src/libutil/signals.cc @@ -118,11 +118,16 @@ void saveSignalMask() { savedSignalMaskIsSet = true; } -void startSignalHandlerThread() +void startSignalHandlerThread(DoSignalSave doSave) { updateWindowSize(); - saveSignalMask(); + switch (doSave) { + case DoSignalSave::Save: + saveSignalMask(); + break; + case DoSignalSave::DontSaveBecauseAdvancedProcess: break; + } sigset_t set; sigemptyset(&set); diff --git a/src/libutil/signals.hh b/src/libutil/signals.hh index 538ff94b4..10ee38e60 100644 --- a/src/libutil/signals.hh +++ b/src/libutil/signals.hh @@ -1,5 +1,23 @@ #pragma once -/// @file +/** @file Signal handling in Lix + * + * Processes are expected to be either: + * - Advanced processes which call into Lix's logic, like the daemon processes. + * - Basic processes that are just going to execve. + * + * Processes should be set up accordingly following a fork: + * In the first case, such processes should have a signal handler thread that + * catches SIGINT and dispatches it to the rest of the system so they should + * call startSignalHandlerThread(). In the second case, processes should call + * restoreProcessContext(), possibly with `false` (depends on whether mounts + * should be restored), which will unmask SIGINT and other signals that were + * previously masked in an advanced process such as the one that started + * them, so the process can be interrupted. + * + * It is generally a mistake to fork a process without at least calling + * restoreSignals() or restoreProcessContext(). + */ + #include "error.hh" @@ -36,13 +54,31 @@ MakeError(Interrupted, BaseError); void restoreSignals(); +/** + * Whether to save the signal mask when starting the signal handler thread. + * + * The signal mask shouldn't be saved if the current signal mask is the one for + * processes with a signal handler thread. + */ +enum class DoSignalSave +{ + Save, + DontSaveBecauseAdvancedProcess, +}; + /** * Start a thread that handles various signals. Also block those signals * on the current thread (and thus any threads created by it). - * Saves the signal mask before changing the mask to block those signals. - * See saveSignalMask(). + * + * Optionally saves the signal mask before changing the mask to block those + * signals. See saveSignalMask(). + * + * This should also be executed after certain forks from Lix processes that + * expect to be "advanced" (see file doc comment), since the signal thread will + * die on fork. Of course this whole situation is kind of unsound since we + * definitely violate async-signal-safety requirements, but, well. */ -void startSignalHandlerThread(); +void startSignalHandlerThread(DoSignalSave doSave = DoSignalSave::Save); /** * Saves the signal mask, which is the signal mask that nix will restore diff --git a/src/nix/daemon.cc b/src/nix/daemon.cc index e1d183d7b..4b84d19eb 100644 --- a/src/nix/daemon.cc +++ b/src/nix/daemon.cc @@ -356,6 +356,10 @@ static void daemonLoop(std::optional forceTrustClientOpt) if (setsid() == -1) throw SysError("creating a new session"); + // Restart the signal handler thread since it met its untimely + // demise at fork time. + startSignalHandlerThread(DoSignalSave::DontSaveBecauseAdvancedProcess); + // Restore normal handling of SIGCHLD. setSigChldAction(false);