Skip to content

Comments

Add explicit termination signal handlers#6

Merged
sysvinit merged 1 commit intowireappfrom
sysvinit/sigterm-handlers
Jun 9, 2022
Merged

Add explicit termination signal handlers#6
sysvinit merged 1 commit intowireappfrom
sysvinit/sigterm-handlers

Conversation

@sysvinit
Copy link

@sysvinit sysvinit commented Jun 8, 2022

Coturn running inside the Docker container runs as PID 1, however PID 1 has special signal handling semantics (see the note at the bottom of the section here). As coturn does not explicitly install signal handlers for SIGTERM (and relies on the default signal behaviour to terminate the process), this means that coturn running as PID 1 does not respond to this signal, which means that it will not terminate when this signal is sent by the container runtime.

This change adds explicit signal handlers for SIGTERM and SIGINT which trigger the same termination mechanism as the admin interface "halt" command.

This is required in order for these signals to be received properly when coturn
is running as pid 1 inside a container, as signal handling for pid 1 (in any
pid namespace) is special.
@sysvinit sysvinit requested review from flokli and supersven June 8, 2022 14:55
Copy link
Collaborator

@supersven supersven left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@sysvinit sysvinit merged commit 9cedf39 into wireapp Jun 9, 2022
@sysvinit sysvinit deleted the sysvinit/sigterm-handlers branch June 9, 2022 09:51
sysvinit pushed a commit that referenced this pull request Nov 8, 2022
```
==6418==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x4e7530 in bcmp /src/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc:906:10
    #1 0x55463d in stun_check_message_integrity_by_key_str coturn/src/client/ns_turn_msg.c:1989:5
    #2 0x554acc in stun_check_message_integrity_str coturn/src/client/ns_turn_msg.c:2008:9
    #3 0x5358c0 in LLVMFuzzerTestOneInput coturn/fuzz/FuzzStun.c:37:5
    #4 0x43ede3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15
    #5 0x42a542 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:324:6
    #6 0x42fdec in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:860:9
    #7 0x459322 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #8 0x7f4cb21790b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/libc-start.c:308:16
    #9 0x42070d in _start
  Uninitialized value was created by an allocation of 'new_hmac' in the stack frame of function 'stun_check_message_integrity_by_key_str'
    #0 0x5538c0 in stun_check_message_integrity_by_key_str coturn/src/client/ns_turn_msg.c:1927
```
sysvinit added a commit that referenced this pull request Nov 8, 2022
This is required in order for these signals to be received properly when coturn
is running as pid 1 inside a container, as signal handling for pid 1 (in any
pid namespace) is special.
eakraly pushed a commit to coturn/coturn that referenced this pull request Dec 7, 2022
coturn running inside a docker container runs as PID 1, however PID 1
has special signal handling semantics (see the note at the bottom of the
section
[here](https://docs.docker.com/engine/reference/run/#foreground)).
coturn relies on the default behaviour of SIGTERM to terminate the
process, however as no signal handler is explicitly installed, it
doesn't respond to SIGTERM when running inside a container. This PR
fixes this problem by installing explicit signal handlers for SIGINT and
SIGTERM, which trigger the same termination mechanism as the admin
interface "halt" command.

This is a port of wireapp#6 for upstream.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants