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

Issue 809721 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: ----
Type: ----



Sign in to add a comment

Chrome crashes if U2fBleDiscovery is destroyed too quickly

Project Member Reported by kpaulhamus@chromium.org, Feb 6 2018

Issue description

Calling credentials.container.create({publicKey}) or get({publicKey}) with a one second timeout leads to this crash, at least on Linux. I found this accidentally, as a one second timeout isn't really reasonable in the real world, but we should handle this gracefully.

[16219:16219:0206/140613.467034:FATAL:scoped_refptr.h(219)] Check failed: ptr_. 
#0 0x7fdf06a0463c base::debug::StackTrace::StackTrace()
#1 0x7fdf06a2eccc logging::LogMessage::~LogMessage()
#2 0x7fdf046082e2 device::U2fBleDiscovery::~U2fBleDiscovery()
#3 0x7fdf0460838e device::U2fBleDiscovery::~U2fBleDiscovery()
#4 0x7fdf046103db device::U2fRequest::~U2fRequest()
#5 0x7fdf0460f31e device::U2fRegister::~U2fRegister()
#6 0x7fdf0450111b content::AuthenticatorImpl::OnTimeout()
#7 0x7fdf06abf3e8 base::Timer::RunScheduledTask()
#8 0x7fdf06a04f5a base::debug::TaskAnnotator::RunTask()
#9 0x7fdf06a397a6 base::internal::IncomingTaskQueue::RunTask()
#10 0x7fdf06a3da87 base::MessageLoop::RunTask()
#11 0x7fdf06a3dea4 base::MessageLoop::DeferOrRunPendingTask()
#12 0x7fdf06a3e474 base::MessageLoop::DoDelayedWork()
#13 0x7fdf06a3feb5 base::(anonymous namespace)::WorkSourceDispatch()
#14 0x7fdefbfa77f7 g_main_context_dispatch
#15 0x7fdefbfa7a60 <unknown>
#16 0x7fdefbfa7b0c g_main_context_iteration
#17 0x7fdf06a3fbc2 base::MessagePumpGlib::Run()
#18 0x7fdf06a3d2dc base::MessageLoop::Run()
#19 0x7fdf06a75ac6 base::RunLoop::Run()
#20 0x55df9a12bd47 ChromeBrowserMainParts::MainMessageLoopRun()
#21 0x7fdf03f68c37 content::BrowserMainLoop::RunMainMessageLoopParts()
#22 0x7fdf03f6c353 content::BrowserMainRunnerImpl::Run()
#23 0x7fdf03f64a5a content::BrowserMain()
#24 0x7fdf049c533b content::RunNamedProcessTypeMain()
#25 0x7fdf049c6487 content::ContentMainRunnerImpl::Run()
#26 0x7fdf06f17614 service_manager::Main()
#27 0x7fdf049c47d1 content::ContentMain()
#28 0x55df99bcf1cb ChromeMain
#29 0x7fdefa6a22b1 __libc_start_main
#30 0x55df99bcf02a _start

Received signal 6
#0 0x7fdf06a0463c base::debug::StackTrace::StackTrace()
#1 0x7fdf06a04111 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7fdf06b5c0c0 <unknown>
#3 0x7fdefa6b4fcf gsignal
#4 0x7fdefa6b63fa abort
#5 0x7fdf06a018c5 base::debug::BreakDebugger()
#6 0x7fdf06a2f0d9 logging::LogMessage::~LogMessage()
#7 0x7fdf046082e2 device::U2fBleDiscovery::~U2fBleDiscovery()
#8 0x7fdf0460838e device::U2fBleDiscovery::~U2fBleDiscovery()
#9 0x7fdf046103db device::U2fRequest::~U2fRequest()
#10 0x7fdf0460f31e device::U2fRegister::~U2fRegister()
#11 0x7fdf0450111b content::AuthenticatorImpl::OnTimeout()
#12 0x7fdf06abf3e8 base::Timer::RunScheduledTask()
#13 0x7fdf06a04f5a base::debug::TaskAnnotator::RunTask()
#14 0x7fdf06a397a6 base::internal::IncomingTaskQueue::RunTask()
#15 0x7fdf06a3da87 base::MessageLoop::RunTask()
#16 0x7fdf06a3dea4 base::MessageLoop::DeferOrRunPendingTask()
#17 0x7fdf06a3e474 base::MessageLoop::DoDelayedWork()
#18 0x7fdf06a3feb5 base::(anonymous namespace)::WorkSourceDispatch()
#19 0x7fdefbfa77f7 g_main_context_dispatch
#20 0x7fdefbfa7a60 <unknown>
#21 0x7fdefbfa7b0c g_main_context_iteration
#22 0x7fdf06a3fbc2 base::MessagePumpGlib::Run()
#23 0x7fdf06a3d2dc base::MessageLoop::Run()
#24 0x7fdf06a75ac6 base::RunLoop::Run()
#25 0x55df9a12bd47 ChromeBrowserMainParts::MainMessageLoopRun()
#26 0x7fdf03f68c37 content::BrowserMainLoop::RunMainMessageLoopParts()
#27 0x7fdf03f6c353 content::BrowserMainRunnerImpl::Run()
#28 0x7fdf03f64a5a content::BrowserMain()
#29 0x7fdf049c533b content::RunNamedProcessTypeMain()
#30 0x7fdf049c6487 content::ContentMainRunnerImpl::Run()
#31 0x7fdf06f17614 service_manager::Main()
#32 0x7fdf049c47d1 content::ContentMain()
#33 0x55df99bcf1cb ChromeMain
#34 0x7fdefa6a22b1 __libc_start_main
#35 0x55df99bcf02a _start
  r8: 0000000000000000  r9: 00007fff85ab4b10 r10: 0000000000000008 r11: 0000000000000246
 r12: 00007fff85ab5218 r13: 00007fff85ab5208 r14: 00007fff85ab5210 r15: 00007fff85ab4da9
  di: 0000000000000002  si: 00007fff85ab4b10  bp: 00007fff85ab4d50  bx: 0000000000000006
  dx: 0000000000000000  ax: 0000000000000000  cx: 00007fdefa6b4fcf  sp: 00007fff85ab4b88
  ip: 00007fdefa6b4fcf efl: 0000000000000246 cgf: 002b000000000033 erf: 0000000000000000
 trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000
[end of stack trace]
Calling _exit(1). Core file will not be generated.

 
It seems to be because U2fBleDiscovery doesn't have an |adapter_| set yet, which causes a crash on teardown.

My linux desktop doesn't have BLE, and it takes about 15 seconds to figure out that it doesn't. After that point, once these logs show up, Chrome no longer crashes:

[66364:66364:0206/154338.648000:ERROR:device_event_log_impl.cc(156)] [15:43:38.647] Bluetooth: bluetooth_adapter_bluez.cc:365 SetPowered: 1. Not Present!
[66364:66364:0206/154338.648033:ERROR:u2f_ble_discovery.cc(91)] Failed to power on the adapter.

(While looking into this, I also noticed that U2fRequest::DiscoveryStarted takes in a (bool success) but doesn't do anything with it. Is there a reason for that?
https://cs.chromium.org/chromium/src/device/fido/u2f_request.cc?l=109)
Yes, those are all good points. U2fBleDiscovery's destructor should check whether the adapter is initialized, and DiscoveryStarted should respect the boolean argument, likely setting the state to DEVICE_ERROR if success == false. I will fix this and add tests.
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 13 2018

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

commit 2b50a91802ba2545b3c0a67f65a082b20b30bc1b
Author: jdoerrie <jdoerrie@chromium.org>
Date: Tue Feb 13 15:54:59 2018

[fido] Improve Robustness of U2fBleDiscovery and U2fRequest

This change improves the robustness of U2fBleDiscovery by checking for
adapter presence in the destructor and modifies U2fRequest to respect
the success argument for DiscoveryStarted.

Bug:  809721 
Change-Id: I889514a0a02948913f7475ca5b142feeebe5fa25
Reviewed-on: https://chromium-review.googlesource.com/915344
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536363}
[modify] https://crrev.com/2b50a91802ba2545b3c0a67f65a082b20b30bc1b/device/fido/u2f_ble_discovery.cc
[modify] https://crrev.com/2b50a91802ba2545b3c0a67f65a082b20b30bc1b/device/fido/u2f_ble_discovery_unittest.cc
[modify] https://crrev.com/2b50a91802ba2545b3c0a67f65a082b20b30bc1b/device/fido/u2f_request.cc
[modify] https://crrev.com/2b50a91802ba2545b3c0a67f65a082b20b30bc1b/device/fido/u2f_request.h
[modify] https://crrev.com/2b50a91802ba2545b3c0a67f65a082b20b30bc1b/device/fido/u2f_request_unittest.cc
[modify] https://crrev.com/2b50a91802ba2545b3c0a67f65a082b20b30bc1b/device/fido/u2f_sign_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment