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

Issue 754764 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Last visit 15 days ago
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

chapsd crashes on stop

Project Member Reported by apronin@chromium.org, Aug 11 2017

Issue description

'stop chapsd' leads to

2017-08-11T09:48:10.712976-07:00 CRIT chapsd[10041]: Check failed: origin_thread_id_ == base::PlatformThread::CurrentId() (10042 vs. 10041)#012/usr/lib64/libbase-core-395517.so(base::debug::StackTrace::StackTrace()+0x13) [0x7b03879dfc33]#012
2017-08-11T09:48:10.750732-07:00 WARNING crash_reporter[10369]: Could not load the device policy file.
2017-08-11T09:48:10.751060-07:00 WARNING crash_reporter[10369]: [user] Received crash notification for chapsd[10041] sig 6, user 223 (developer build - not testing - always dumping)
2017-08-11T09:48:10.754431-07:00 INFO crash_reporter[10369]: State of crashed process [10041]: S (sleeping)

Looks like one of the global objects initialized in a worker thread being destroyed in the main thread when the daemon exits. Possibly caused by changes in https://crrev.com/c/520163 - one/some of the objects created in OnInit(), which is called by the DBusServiceDaemon thread. Should destroy those tpm_/factory_/slot_manager_/service_ in OnShutdown().
 
Components: Internals
Cc: -ejcaruso@chromium.org apronin@chromium.org
Owner: ejcaruso@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/464ceee52b908996896b94fa73d53ed3f5f64c8d

commit 464ceee52b908996896b94fa73d53ed3f5f64c8d
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Fri Aug 18 21:27:23 2017

chaps: destroy sub-objects on correct thread

TrunksDBusProxy expects to have its API used by exactly one
thread. Unfortunately while this is not explicitly stated the
destructor also must be called from this one thread because it
shuts down a dbus::Bus. Because of this, the owning object
(TPM2UtilityImpl) should keep a reference to the task runner of
the thread which initialized the TrunksDBusProxy and use it
to clean up the proxy when it is being destroyed.

Because this restriction requires the task runner passed to
TPM2UtilityImpl to always use the same thread, we also need to
change the task runner it expects to be a SingleThreadTaskRunner.
This is fine because that's the only type of task runner we ever
used with it anyway.

BUG= chromium:754764 
TEST=stop chaps and verify no crashes in /var/log/messages on eve;
  use pkcs11-tool while repeatedly restarting chapsd and ensure
  calls either work correctly or fail if they happened during a
  restart

Change-Id: Ie9124c1ee76029d4ff43f4c1398c7679e6fc37e9
Reviewed-on: https://chromium-review.googlesource.com/614121
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/464ceee52b908996896b94fa73d53ed3f5f64c8d/chaps/tpm2_utility_impl.cc
[modify] https://crrev.com/464ceee52b908996896b94fa73d53ed3f5f64c8d/chaps/tpm2_utility_impl.h

Status: Fixed (was: Started)
Likely a dup/related issue 764027.
Labels: Merge-Request-61
Requesting merge to eliminate a source of crashes in 61. See also https://crbug.com/764027#c8.
Project Member

Comment 7 by sheriffbot@chromium.org, Sep 11 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: keta...@chromium.org
Labels: -Merge-Review-61 Merge-Approved-61 Merge-Approved-62
Approving merge to M61 and M62.
Labels: -Merge-Approved-62
Landed in 9856, so is already in M62.
Project Member

Comment 11 by bugdroid1@chromium.org, Sep 13 2017

Labels: merge-merged-release-R61-9765.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/18d9b4cdb7cb8059b29260893e4be9cf5cf6ec2e

commit 18d9b4cdb7cb8059b29260893e4be9cf5cf6ec2e
Author: Eric Caruso <ejcaruso@chromium.org>
Date: Wed Sep 13 22:34:55 2017

chaps: destroy sub-objects on correct thread

TrunksDBusProxy expects to have its API used by exactly one
thread. Unfortunately while this is not explicitly stated the
destructor also must be called from this one thread because it
shuts down a dbus::Bus. Because of this, the owning object
(TPM2UtilityImpl) should keep a reference to the task runner of
the thread which initialized the TrunksDBusProxy and use it
to clean up the proxy when it is being destroyed.

Because this restriction requires the task runner passed to
TPM2UtilityImpl to always use the same thread, we also need to
change the task runner it expects to be a SingleThreadTaskRunner.
This is fine because that's the only type of task runner we ever
used with it anyway.

BUG= chromium:754764 
TEST=stop chaps and verify no crashes in /var/log/messages on eve;
  use pkcs11-tool while repeatedly restarting chapsd and ensure
  calls either work correctly or fail if they happened during a
  restart

Change-Id: Ie9124c1ee76029d4ff43f4c1398c7679e6fc37e9
Reviewed-on: https://chromium-review.googlesource.com/614121
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>
(cherry picked from commit 464ceee52b908996896b94fa73d53ed3f5f64c8d)
Reviewed-on: https://chromium-review.googlesource.com/665857
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
Commit-Queue: Andrey Pronin <apronin@chromium.org>
Tested-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/18d9b4cdb7cb8059b29260893e4be9cf5cf6ec2e/chaps/tpm2_utility_impl.cc
[modify] https://crrev.com/18d9b4cdb7cb8059b29260893e4be9cf5cf6ec2e/chaps/tpm2_utility_impl.h

Labels: -Merge-Approved-61

Comment 13 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment