New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 687018 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 687022



Sign in to add a comment

trunks: background thread should handle signals

Project Member Reported by apronin@chromium.org, Jan 31 2017

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 +++
========

 
Blocking: 687022
Owner: apronin@chromium.org
Status: Started (was: Untriaged)
Blocking: 684141
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.

Comment 5 Deleted

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.
Blocking: -684141
Status: Fixed (was: Started)
CL https://chromium-review.googlesource.com/#/c/435548/ has actually landed, just not reflected here.
Cc: keta...@chromium.org hennessywill@chromium.org
Labels: Merge-Request-57
Cc: bleung@chromium.org
Status: Started (was: Fixed)
Labels: OS-Chrome
Labels: -Merge-Request-57 Merge-Approved-57
Approving merge to M57 Chrome OS.
Project Member

Comment 13 by bugdroid1@chromium.org, 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

Project Member

Comment 14 by bugdroid1@chromium.org, Feb 7 2017

Labels: merge-merged-release-R57-9202.B
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

Labels: -Merge-Approved-57 Merge-Merged
Status: Fixed (was: Started)

Comment 16 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 17 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 19 by dchan@chromium.org, Oct 14 2017

Status: Archived (was: Fixed)

Sign in to add a comment