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

Issue 875338 link

Starred by 1 user
Status: Fixed
Owner:
martinkr@google.com
Closed: Aug 23
Cc:
engedy@chromium.org
█ hongjunchoi@chromium.org
kenrb@chromium.org
martinkr@google.com
ssoneff@google.com
kpaulhamus@chromium.org
jdoerrie@chromium.org
mknowles@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Callback lifetime bug in MaybeMakePlatformAuthenticator callback

Project Member Reported by martinkr@google.com, Aug 17 
In crrev.com/c/1175418, FidoRequestHandlerBase's invocation of MaybeAddPlatformAuthenticator was made asynchronous. That method invokes a callback which points at AuthenticatorImpl::CreatePlatformAuthenticatorIfAvailable, which is bound with base::Unretained. Binding unretained worked as long as the callback was invoked synchronously with request handler instantiation, but now that it's async it is a potential dangling reference issue.
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 18 

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

commit 7cbada4371e3e906046f998dbf1e967791360b6c
Author: Martin Kreichgauer <martinkr@google.com>
Date: Sat Aug 18 02:30:14 2018

fido: fix lifetime issue with MaybeAddPlatformAuthenticatorCallback

Currently, |AuthenticatorImpl| is binding
|CreatePlatformAuthenticatorIfAvailable| with |base::Unretained| for the
|MaybeAddPlatformAuthenticatorCallback| argument to the
|FidoRequestHandlerBase| ctor. The callback was originally invoked
synchronously from the ctor, but then recently changed to async
invocation to account for observer registration (crrev.com/c/1175418).

This is a potential callback lifetime issue because the
AuthenticatorImpl and its request handler can now theoretically get
destroyed before the callback gets invoked.

To work around this issue, replace the callback with a method on the
request handler invoked by the AuthenticatorImpl.

Bug:  875338 
Change-Id: I089f0783a46791ae8d9d6bac5f0e663e52075b86
Reviewed-on: https://chromium-review.googlesource.com/1179081
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Jun Choi <hongjunchoi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584288}
[modify] https://crrev.com/7cbada4371e3e906046f998dbf1e967791360b6c/content/browser/webauth/authenticator_impl.cc
[modify] https://crrev.com/7cbada4371e3e906046f998dbf1e967791360b6c/device/fido/fido_request_handler.h
[modify] https://crrev.com/7cbada4371e3e906046f998dbf1e967791360b6c/device/fido/fido_request_handler_base.cc
[modify] https://crrev.com/7cbada4371e3e906046f998dbf1e967791360b6c/device/fido/fido_request_handler_base.h
[modify] https://crrev.com/7cbada4371e3e906046f998dbf1e967791360b6c/device/fido/fido_request_handler_unittest.cc
[modify] https://crrev.com/7cbada4371e3e906046f998dbf1e967791360b6c/device/fido/get_assertion_handler_unittest.cc
[modify] https://crrev.com/7cbada4371e3e906046f998dbf1e967791360b6c/device/fido/get_assertion_request_handler.cc
[modify] https://crrev.com/7cbada4371e3e906046f998dbf1e967791360b6c/device/fido/get_assertion_request_handler.h
[modify] https://crrev.com/7cbada4371e3e906046f998dbf1e967791360b6c/device/fido/make_credential_handler_unittest.cc
[modify] https://crrev.com/7cbada4371e3e906046f998dbf1e967791360b6c/device/fido/make_credential_request_handler.cc
[modify] https://crrev.com/7cbada4371e3e906046f998dbf1e967791360b6c/device/fido/make_credential_request_handler.h

Comment 2 by martinkr@google.com, Aug 23

Status: Fixed (was: Started)
►

Sign in to add a comment