Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 60 additions & 1 deletion bubblewrap.c
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ static bool opt_unshare_cgroup_try = false;
static bool opt_needs_devpts = false;
static bool opt_new_session = false;
static bool opt_die_with_parent = false;
static bool opt_forward_signals = false;
static uid_t opt_sandbox_uid = -1;
static gid_t opt_sandbox_gid = -1;
static int opt_sync_fd = -1;
Expand Down Expand Up @@ -373,6 +374,7 @@ usage (int ecode, FILE *out)
" --perms OCTAL Set permissions of next argument (--bind-data, --file, etc.)\n"
" --size BYTES Set size of next argument (only for --tmpfs)\n"
" --chmod OCTAL PATH Change permissions of PATH (must already exist)\n"
" --forward-signals Forward various commonly used signals to the child process.\n"
);
exit (ecode);
}
Expand All @@ -387,6 +389,33 @@ handle_die_with_parent (void)
die_with_error ("prctl");
}

static int forwarded_signals[] =
{
SIGINT,
SIGTERM,
SIGCONT,
SIGHUP,
SIGQUIT,
SIGUSR1,
SIGUSR2,
SIGWINCH,
};

static void
block_forwarded_signals (sigset_t *prevmask)
{
sigset_t mask;
size_t i;

sigemptyset (&mask);

for (i = 0; i < N_ELEMENTS (forwarded_signals); i++)
sigaddset (&mask, forwarded_signals[i]);

if (sigprocmask (SIG_BLOCK, &mask, prevmask) == -1)
die_with_error ("sigprocmask");
}

static void
block_sigchild (void)
{
Expand Down Expand Up @@ -508,6 +537,7 @@ monitor_child (int event_fd, pid_t child_pid, int setup_finished_fd)
int exitc;
pid_t died_pid;
int died_status;
size_t i;

/* Close all extra fds in the monitoring process.
Any passed in fds have been passed on to the child anyway. */
Expand All @@ -523,6 +553,9 @@ monitor_child (int event_fd, pid_t child_pid, int setup_finished_fd)
sigemptyset (&mask);
sigaddset (&mask, SIGCHLD);

for (i = 0; i < N_ELEMENTS (forwarded_signals); i++)
sigaddset (&mask, forwarded_signals[i]);
Comment on lines +556 to +557
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be guarded by if (opt_forward_signals)?


signal_fd = signalfd (-1, &mask, SFD_CLOEXEC | SFD_NONBLOCK);
if (signal_fd == -1)
die_with_error ("Can't create signalfd");
Expand Down Expand Up @@ -561,12 +594,21 @@ monitor_child (int event_fd, pid_t child_pid, int setup_finished_fd)
}

/* We need to read the signal_fd, or it will keep polling as read,
* however we ignore the details as we get them from waitpid
* however we ignore the details for SIGCHLD as we get them from waitpid
* below anyway */
s = read (signal_fd, &fdsi, sizeof (struct signalfd_siginfo));
if (s == -1 && errno != EINTR && errno != EAGAIN)
die_with_error ("read signalfd");

/* Propagate signal to child so that it will take the correct
* action. This avoids the parent terminating, leaving an orphan. */
if (fdsi.ssi_signo != SIGCHLD)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (fdsi.ssi_signo != SIGCHLD)
if (opt_forward_signals && fdsi.ssi_signo != SIGCHLD)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might need a check for child_pid != 0 (or for exec_pid != 0 after the second commit), too.

{
s = kill (child_pid, fdsi.ssi_signo);
if (s != 0 && s != ESRCH)
die_with_error("kill child");
}

/* We may actually get several sigchld compressed into one
SIGCHLD, so we have to handle all of them. */
while ((died_pid = waitpid (-1, &died_status, WNOHANG)) > 0)
Expand Down Expand Up @@ -2742,6 +2784,10 @@ parse_args_recurse (int *argcp,
argc -= 1;
break;
}
else if (strcmp (arg, "--forward-signals") == 0)
{
opt_forward_signals = true;
}
else if (*arg == '-')
{
die ("Unknown option %s", arg);
Expand Down Expand Up @@ -2889,6 +2935,8 @@ main (int argc,
int intermediate_pids_sockets[2] = {-1, -1};
const char *exec_path = NULL;
int i;
sigset_t sigmask_before_forwarding;
sigemptyset (&sigmask_before_forwarding);

/* Handle --version early on before we try to acquire/drop
* any capabilities so it works in a build environment;
Expand Down Expand Up @@ -3062,6 +3110,10 @@ main (int argc,
/* We block sigchild here so that we can use signalfd in the monitor. */
block_sigchild ();

/* We block other signals here to avoid leaving an orphan. */
if (opt_forward_signals)
block_forwarded_signals (&sigmask_before_forwarding);

clone_flags = SIGCHLD | CLONE_NEWNS;
if (opt_unshare_user)
clone_flags |= CLONE_NEWUSER;
Expand Down Expand Up @@ -3212,6 +3264,13 @@ main (int argc,
return monitor_child (event_fd, pid, setup_finished_pipe[0]);
}

/* Restore the state of sigmask from before the blocking. */
if (opt_forward_signals)
{
if (sigprocmask (SIG_SETMASK, &sigmask_before_forwarding, NULL) != 0)
die_with_error ("sigprocmask");
}

if (opt_pidns_fd > 0)
{
if (setns (opt_pidns_fd, CLONE_NEWPID) != 0)
Expand Down
1 change: 1 addition & 0 deletions tests/meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ test_scripts = [
'test-seccomp.py',
'test-specifying-pidns.sh',
'test-specifying-userns.sh',
'test-forward-signals.sh'
]

test_env = environment()
Expand Down
44 changes: 44 additions & 0 deletions tests/test-forward-signals.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#!/usr/bin/env bash

set -xeuo pipefail

srcd=$(cd $(dirname "$0") && pwd)
. ${srcd}/libtest.sh
test_count=0
ok () {
test_count=$((test_count + 1))
echo ok $test_count "$@"
}
ok_skip () {
ok "# SKIP" "$@"
}
done_testing () {
echo "1..$test_count"
}

sh_path=/bin/sh

out=$(
$RUN \
"$sh_path" -c 'trap "echo USR1; exit" USR1; sleep 0.5' &
sleep 0.1
kill -USR1 $!
)
if [ -z "$out" ]; then
ok "No signals forwarded without --forward-signal"
else
false
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false will not give a very comprehensible error message if the test fails. Better to have something like:

Suggested change
false
assert_not_reached "Unexpected output from child, was SIGUSR1 forwarded? $out"

or something along those lines

fi

out=$(
$RUN --forward-signals \
"$sh_path" -c 'trap "echo USR1; exit" USR1; sleep 0.5' &
sleep 0.1
Comment on lines +35 to +36
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An arbitrary sleep in unit tests is race-condition-prone. On slow or heavily-loaded machines, 0.1 seconds might not be long enough for bwrap and sh to work through all of their setup and reach the point after the trap command has been executed.

To make this test reliable, the parent (script) will need to, somehow, wait for the child (bwrapsh) to have put its trap in place before killing the child.

One way to do this (if I'm remembering my sh arcana correctly) would be:

  1. Use mkfifo to create a fifo: mkfifo wait-for-trap or something
  2. In the -c script, after the trap, write something to the fifo: 'trap ...; echo done > wait-for-trap; sleep ...'
  3. In the parent, instead of the sleep 0.1, use something like cat wait-for-trap >/dev/null, which will block until there is a writer

That way, the interactions with wait-for-trap establish a sequence point: the trap command happens before them, and the kill -USR1 happens after.

Similarly, if the host is really heavily loaded, 0.5 seconds might not be long enough for the parent to wake up and send the signal. A sleep of a few seconds should be enough, though.

kill -USR1 $!
)

if [ "$out" = USR1 ]; then
ok "Successfully forwarded signals with --forward-signals"
else
false
fi