trunks: background thread should handle signals |
|||||||||||||||
Issue description
When a trunksd process is stopped (using 'initctl stop trunksd' or by sending a signal) it doesn't go through proper shutdown procedure. Attempting to do anything in hooks like DBusServiceDaemon::OnShutdown() fails - they are simply not called.
A single line "init: trunksd main process (NNN) killed by TERM signal" is added to the logs.
Analysis:
Trunks daemon starts a DBusServiceDaemon instance (for handling D-Bus requests), which registers SIGINT/SIHTERM/SIGHUP signal handlers (in brillo::Daemon::OnInit()) to properly handle shutdowns (and restarts).
However, trunks daemon also starts a background thread for BackgroundCommandTransceiver. This thread doesn't register any signal handlers. As a result, when SIGTERM is received, the process is terminated before DBusServiceDaemon can properly shutdown D-Bus.
Here is what happens on receiving SIGINT when both threads are running (strace -f trunksd):
========
[Main D-Bus thread detects the signal and starts processing it]
[pid 822] <... epoll_wait resumed> [{EPOLLIN, {u32=10, u64=10}}], 32, -1) = 1
[The background thread is also woken by the signal]
[pid 823] <... futex resumed> ) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
[The main D-Bus thread unregisters the signal handler and moves on to doing other cleanup]
[pid 822] epoll_ctl(3, EPOLL_CTL_DEL, 10, 0x7ffcef72a790) = 0
[pid 822] write(7, "\0", 1) = 1
[But the background thread doesn't process SIGINT and the process decides to die]
[pid 823] --- SIGINT {si_signo=SIGINT, si_code=SI_USER, si_pid=1045, si_uid=0} ---
[pid 823] +++ killed by SIGINT +++
+++ killed by SIGINT +++
========
If commenting out the background thread, the main dbus thread shuts down correctly and calls OnShutdown():
========
[The main D-Bus thread wakes up on SIGINT]
epoll_wait(3, [{EPOLLIN, {u32=6, u64=6}}], 32, 0) = 1
read(6, "\0", 1) = 1
write(7, "\0", 1) = 1
[Prints OnShutdown debug message]
sendto(9, "<11>Jan 30 20:08:09 trunksd: OnS"..., 40, MSG_NOSIGNAL, NULL, 0) = 40
[...skipped...]
[Unregisters on D-Bus]
sendmsg(11, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="l\1\0\1\30\0\0\0\3\0\0\0\177\0\0\0\1\1o\0\25\0\0\0/org/fre"..., iov_len=144}, {iov_base="\23\0\0\0org.chromium.Trunks\0", iov_len=24}], msg_iovlen=2, msg_controllen=0, msg_flags=0}, MSG_NOSIGNAL) = 168
[...skipped...]
[Unregisters the signal handlers]
epoll_ctl(3, EPOLL_CTL_DEL, 10, 0x7ffd75d4e290) = 0
close(10) = 0
rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
[...skipped...]
[Closes /dev/tpm0]
epoll_ctl(3, EPOLL_CTL_DEL, 6, 0x7ffd75d4db60) = 0
[...skipped...]
[Exits normally]
exit_group(0) = ?
+++ exited with 0 +++
========
,
Jan 31 2017
,
Jan 31 2017
,
Jan 31 2017
Looks like the issue is that the background thread is forked before we set the sigmask in DBusServiceDaemon, so it remains empty there. Need to set a sigmask either before starting the thread or for the thread itself.
,
Jan 31 2017
TrunksBinderService, which is used instead of TrunksDBusService in case of USE_BINDER_IPC (on Android), is also a brillo::Daemon, and as such also sets signal handlers and mask for TERM, INT and HUP to properly handle shutdown. So, the background thread should block these signals in both Binder and D-Bus cases.
,
Feb 1 2017
,
Feb 4 2017
CL https://chromium-review.googlesource.com/#/c/435548/ has actually landed, just not reflected here.
,
Feb 6 2017
,
Feb 6 2017
,
Feb 6 2017
,
Feb 7 2017
Approving merge to M57 Chrome OS.
,
Feb 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/aosp/platform/system/tpm/+/45fb72def24bc3e9df7eadbfb88bfe74a989cfc8 commit 45fb72def24bc3e9df7eadbfb88bfe74a989cfc8 Author: Andrey Pronin <apronin@chromium.org> Date: Fri Feb 03 07:31:13 2017 trunks: mask signals handled by the service in all threads The service object started by trunksd installs its own signal handlers to perform graceful shutdown. Before this change, the background thread was started with an empty signal mask, which resulted in the process killed before it could finish the graceful shutdown on the service thread. This change masks the signal handled by the service before starting the background thread. The thread then inherits the signal mask. BUG= chromium:687018 TEST=with a running trunksd, run "kill `pidof trunksd`", check that shutdown messages are present in the log. Change-Id: Ib617c04ccdf99a38f055f1014b045599475bfb94 Reviewed-on: https://chromium-review.googlesource.com/435548 Commit-Ready: Andrey Pronin <apronin@chromium.org> Tested-by: Andrey Pronin <apronin@chromium.org> Reviewed-by: Darren Krahn <dkrahn@chromium.org> [modify] https://crrev.com/45fb72def24bc3e9df7eadbfb88bfe74a989cfc8/trunks/trunksd.cc
,
Feb 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/aosp/platform/system/tpm/+/4a48dd76a4a01c0c0a1f5934c68d79a8318d873b commit 4a48dd76a4a01c0c0a1f5934c68d79a8318d873b Author: Andrey Pronin <apronin@chromium.org> Date: Tue Feb 07 02:37:24 2017 trunks: mask signals handled by the service in all threads The service object started by trunksd installs its own signal handlers to perform graceful shutdown. Before this change, the background thread was started with an empty signal mask, which resulted in the process killed before it could finish the graceful shutdown on the service thread. This change masks the signal handled by the service before starting the background thread. The thread then inherits the signal mask. BUG= chromium:687018 TEST=with a running trunksd, run "kill `pidof trunksd`", check that shutdown messages are present in the log. Change-Id: Ib617c04ccdf99a38f055f1014b045599475bfb94 Reviewed-on: https://chromium-review.googlesource.com/435548 Commit-Ready: Andrey Pronin <apronin@chromium.org> Tested-by: Andrey Pronin <apronin@chromium.org> Reviewed-by: Darren Krahn <dkrahn@chromium.org> (cherry picked from commit 45fb72def24bc3e9df7eadbfb88bfe74a989cfc8) [modify] https://crrev.com/4a48dd76a4a01c0c0a1f5934c68d79a8318d873b/trunks/trunksd.cc
,
Feb 7 2017
,
Apr 17 2017
,
May 30 2017
,
Aug 1 2017
,
Oct 14 2017
|
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by apronin@chromium.org
, Jan 31 2017