Skip to content

Commit

Permalink
Use synchronous signal handling in container mode.
Browse files Browse the repository at this point in the history
Inspired by Yelp/dumb-init#72.
In container mode, we need to have a process that acts as PID 1
and forwards all signals to the actual benchmarked process
(while at the same time waiting for this process to finish).
We can do this with sigwait instead of asynchronous signal handlers,
which is less and complex and avoids some signal handling problems.
  • Loading branch information
PhilippWendler committed Oct 17, 2016
1 parent 916416a commit dc5d11a
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 12 deletions.
54 changes: 48 additions & 6 deletions benchexec/container.py
Original file line number Diff line number Diff line change
Expand Up @@ -287,15 +287,26 @@ def drop_capabilities():
ctypes.byref((libc.CapData * 2)()))


_ALL_SIGNALS = range(1, signal.NSIG)
_FORWARDABLE_SIGNALS = set(range(1, 32)).difference([signal.SIGKILL, signal.SIGSTOP, signal.SIGCHLD])
_HAS_SIGWAIT = hasattr(signal, 'sigwait')

def forward_all_signals(target_pid, process_name):
def block_all_signals():
"""Block asynchronous delivery of all signals to this process."""
if _HAS_SIGWAIT:
signal.pthread_sigmask(signal.SIG_BLOCK, _ALL_SIGNALS)

def _forward_signal(signum, target_pid, process_name):
logging.debug("Forwarding signal %d to process %s.", signum, process_name)
try:
os.kill(target_pid, signum)
except OSError as e:
logging.debug("Could not forward signal %d to process %s: %s", signum, process_name, e)

def forward_all_signals_async(target_pid, process_name):
"""Install all signal handler that forwards all signals to the given process."""
def forwarding_signal_handler(signum):
logging.debug("Forwarding signal %d to process %s.", signum, process_name)
try:
os.kill(forwarding_signal_handler.target_pid, signum)
except OSError as e:
logging.debug("Could not forward signal %d to process %s: %s", signum, process_name, e)
_forward_signal(signum, process_name, forwarding_signal_handler.target_pid)

# Somehow we get a Python SystemError sometimes if we access target_pid directly from inside function.
forwarding_signal_handler.target_pid = target_pid
Expand All @@ -306,6 +317,37 @@ def forwarding_signal_handler(signum):
# (it may think we are in a different thread than the main thread).
libc.signal(signum, forwarding_signal_handler)

# Reactivate delivery of signals such that our handler gets called.
reset_signal_handling()

def wait_for_child_and_forward_all_signals(child_pid, process_name):
"""Wait for a child to terminate and in the meantime forward all signals the current process
receives to this child.
@return a tuple of exit code and resource usage of the child as given by os.waitpid
"""
assert _HAS_SIGWAIT
block_all_signals()

while True:
logging.debug("Waiting for signals")
signum = signal.sigwait(_ALL_SIGNALS)
if signum == signal.SIGCHLD:
pid, exitcode, ru_child = os.wait4(-1, os.WNOHANG)
while pid != 0:
if pid == child_pid:
return exitcode, ru_child
else:
logging.debug("Received unexpected SIGCHLD for PID %s", pid)
pid, exitcode, ru_child = os.wait4(-1, os.WNOHANG)

else:
_forward_signal(signum, child_pid, process_name)

def reset_signal_handling():
if _HAS_SIGWAIT:
signal.pthread_sigmask(signal.SIG_SETMASK, {})


def close_open_fds(keep_files=[]):
"""Close all open file descriptors except those in a given set.
@param keep_files: an iterable of file descriptors or file-like objects.
Expand Down
21 changes: 15 additions & 6 deletions benchexec/containerexecutor.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
DIR_FULL_ACCESS = "full-access"
DIR_MODES = [DIR_HIDDEN, DIR_READ_ONLY, DIR_OVERLAY, DIR_FULL_ACCESS]

_HAS_SIGWAIT = hasattr(signal, 'sigwait')

def add_basic_container_args(argument_parser):
argument_parser.add_argument("--network-access", action="store_true",
Expand Down Expand Up @@ -411,6 +412,7 @@ def grandchild():

container.mount_proc()
container.drop_capabilities()
container.reset_signal_handling()
child_setup_fn() # Do some other setup the caller wants.

# Signal readiness to parent by sending our PID and wait until parent is also ready
Expand All @@ -429,6 +431,9 @@ def child():
logging.debug("Child: child process of RunExecutor with PID %d started",
container.get_my_pid_from_procfs())

# Put all received signals on hold until we handle them later.
container.block_all_signals()

# We want to avoid leaking file descriptors to the executed child.
# It is also nice if the child has only the minimal necessary file descriptors,
# to avoid keeping other pipes and files open, e.g., those that the parent
Expand Down Expand Up @@ -468,16 +473,20 @@ def child():

container.drop_capabilities()

# Close other fds that were still necessary above.
container.close_open_fds(keep_files={sys.stdout, sys.stderr, to_parent})

# Set up signal handlers to forward signals to grandchild
# (because we are PID 1, there is a special signal handling otherwise).
# cf. dumb-init project: https://github.com/Yelp/dumb-init
container.forward_all_signals(grandchild_proc.pid, args[0])

# Close other fds that were still necessary above.
container.close_open_fds(keep_files={sys.stdout, sys.stderr, to_parent})
# Also wait for grandchild and return its result.
if _HAS_SIGWAIT:
grandchild_result = container.wait_for_child_and_forward_all_signals(
grandchild_proc.pid, args[0])
else:
container.forward_all_signals_async(grandchild_proc.pid, args[0])
grandchild_result = self._wait_for_process(grandchild_proc.pid, args[0])

# wait for grandchild and return its result
grandchild_result = self._wait_for_process(grandchild_proc.pid, args[0])
logging.debug("Child: process %s terminated with exit code %d.",
args[0], grandchild_result[0])
os.write(to_parent, pickle.dumps(grandchild_result))
Expand Down

0 comments on commit dc5d11a

Please sign in to comment.