New issue
Advanced search Search tips

Issue 711207 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Chrome crashes on login

Project Member Reported by nya@chromium.org, Apr 13 2017

Issue description

Chrome OS Chrome occasionally crashes on login. This was caught by cheets_CTSHelper.stress, an autotest to repeat logging in 10 times.

Short stack trace of crashing UI thread is below. Full stack trace on crash is attached.

Thread 0 (crashed)
 0  libnss3.so!<name omitted> [pk11slot.c : 477 + 0x0]
    rax = 0x00007f6ab6fd473f   rdx = 0x00000de713656420
    rcx = 0xe19351a68f5a2600   rbx = 0x0000000000000000
    rsi = 0x0000000000000000   rdi = 0x0000000000000000
    rbp = 0x00007f6ab6fd4770   rsp = 0x00007f6ab6fd46d8
     r8 = 0x0000000000000000    r9 = 0x0000000000000128
    r10 = 0x00000000684e884b   r11 = 0x0000000000000068
    r12 = 0x00000de713de6820   r13 = 0x00000de713a0a000
    r14 = 0x0000000000000000   r15 = 0x0000000000000000
    rip = 0x00007f6ac6a50550
    Found by: given as instruction pointer in context
 1  libnss3.so!PK11_SignWithMechanism [pk11obj.c : 171 + 0xc]
    rbx = 0x0000000000000000   rbp = 0x00007f6ab6fd4770
    rsp = 0x00007f6ab6fd46e0   r12 = 0x00000de713de6820
    r13 = 0x00000de713a0a000   r14 = 0x0000000000000000
    r15 = 0x0000000000000000   rip = 0x00007f6ac6a43b6a
    Found by: call frame info
 2  libnss3.so!SGN_End [secsign.c : 203 + 0x8]
    rbx = 0x00000de713cc5c80   rbp = 0x00007f6ab6fd4830
    rsp = 0x00007f6ab6fd4780   r12 = 0x00007f6ab6fd4848
    r13 = 0x00000de713de6820   r14 = 0x00000de711b66280
    r15 = 0x0000000000000000   rip = 0x00007f6ac6a280a7
    Found by: call frame info
 3  chrome!AssembleAndSignPolicy [owner_settings_service.cc : 57 + 0x8]
    rbx = 0x00000de71348ba50   rbp = 0x0000610a3acee068
    rsp = 0x00007f6ab6fd4840   r12 = 0x00000de7140a9150
    r13 = 0x00000de7140a9178   r14 = 0x00007f6ab6fd4bc0
    r15 = 0x00000de713cc5c80   rip = 0x0000610a37c576bc
    Found by: call frame info
 4  chrome!Run [bind_internal.h : 164 + 0x5]
    rbx = 0x00007f6ab6fd4bc0   rbp = 0x00007f6ab6fd4de0
    rsp = 0x00007f6ab6fd4ba0   r12 = 0x0000610a341f3d30
    r13 = 0x00000de71097dc00   r14 = 0x00007f6ab6fd4bc8
    r15 = 0x00007f6ab6fd4bf8   rip = 0x0000610a37c57c00
    Found by: call frame info

I'm suspecting my recent change https://codereview.chromium.org/2761353003.

Internal bug: b/37294435

 
stacktrace.txt
165 KB View Download

Comment 1 by nya@chromium.org, Apr 13 2017

Cc: xiy...@chromium.org
Hm, there was another recent change to the same file https://codereview.chromium.org/2796303005/ . Adding xiyuan@ just in case.

Comment 2 by xiy...@chromium.org, Apr 13 2017

I suspect the crash is caused by a released |private_key|. I noticed there could be multiple ReloadKeypair calls (esp. for the first signed-in user, one from OnTPMTokenReady and one from OwnerKeySet). The crash could happen when they happen to interleave with a AssembleAndSignPolicy.

A possible fix would be using a scoped_refptr<PrivateKey> as arg to AssembleAndSignPolicy so that PrivateKey does not get released under the call. Think this should be fine because even though ReloadKeypair creates differernt PrivateKey instance, they should refer to the same private key and equivalent for signing purpose.

Comment 3 by nya@chromium.org, Apr 17 2017

Labels: -Pri-2 M-59 Pri-1
xiyuan: Thanks, that sounds right. I'll create a CL and send it to you.

Since this is a crash bug I'm increasing priority and targetting to cherry-pick to M59.

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/20138f3d0b1f72eb232363cac921dfc8c21812cf

commit 20138f3d0b1f72eb232363cac921dfc8c21812cf
Author: nya <nya@chromium.org>
Date: Tue Apr 18 01:54:45 2017

Fix crash on login.

When OwnerSettingsService::ReloadKeypair() is called, |private_key_| is
replaced with a new value. Thus a reference must be retained when we use
it in another thread.

BUG= chromium:711207 
TEST=test_that samus tmp_LoginStressTest
     # https://chromium-review.googlesource.com/c/458138/

Review-Url: https://codereview.chromium.org/2818383002
Cr-Commit-Position: refs/heads/master@{#465096}

[modify] https://crrev.com/20138f3d0b1f72eb232363cac921dfc8c21812cf/components/ownership/owner_settings_service.cc

Comment 6 by nya@chromium.org, Apr 18 2017

Labels: Merge-Request-59
Status: Fixed (was: Started)

Comment 7 by gkihumba@google.com, Apr 18 2017

Labels: Merge-Approved-59
Project Member

Comment 8 by sheriffbot@chromium.org, Apr 19 2017

Labels: -Merge-Request-59 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 19 2017

Labels: -merge-approved-59 merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4c4a23513a0410c13828142b43b335983598acb8

commit 4c4a23513a0410c13828142b43b335983598acb8
Author: Shuhei Takahashi <nya@chromium.org>
Date: Wed Apr 19 04:15:35 2017

Fix crash on login.

When OwnerSettingsService::ReloadKeypair() is called, |private_key_| is
replaced with a new value. Thus a reference must be retained when we use
it in another thread.

BUG= chromium:711207 
TEST=test_that samus tmp_LoginStressTest
     # https://chromium-review.googlesource.com/c/458138/

Review-Url: https://codereview.chromium.org/2818383002
Cr-Commit-Position: refs/heads/master@{#465096}
(cherry picked from commit 20138f3d0b1f72eb232363cac921dfc8c21812cf)

Review-Url: https://codereview.chromium.org/2828593003 .
Cr-Commit-Position: refs/branch-heads/3071@{#44}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/4c4a23513a0410c13828142b43b335983598acb8/components/ownership/owner_settings_service.cc

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

Status: Archived (was: Fixed)

Sign in to add a comment