New issue
Advanced search Search tips

Issue 903294 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 20
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

device_unittests FidoRequestHandlerTest fail under UBSan

Project Member Reported by h...@chromium.org, Nov 8

Issue description

Example build:
https://ci.chromium.org/buildbot/chromium.clang/ToTLinuxUBSanVptr/4504

These tests fail:
FidoRequestHandlerTest.TestRequestWithMultipleFailureResponses
FidoRequestHandlerTest.TestRequestWithOperationDeniedErrorInternalTransport
FidoRequestHandlerTest.TestSingleDeviceSuccess
FidoRequestHandlerTest.TestRequestWithMultipleSuccessResponses
FidoRequestHandlerTest.TestAuthenticatorHandlerReset
FidoRequestHandlerTest.TestRequestWithOperationDeniedErrorCrossPlatform
FidoRequestHandlerTest.TestRequestWithMultipleDevices

Test output from the first one:

[ RUN      ] FidoRequestHandlerTest.TestRequestWithMultipleFailureResponses
../../device/fido/fido_request_handler_unittest.cc:193:5: runtime error: downcast of address 0x10f2ba200a50 which does not point to an object of type 'device::(anonymous namespace)::FakeFidoAuthenticator'
0x10f2ba200a50: note: object is of type 'device::FidoDeviceAuthenticator'
 65 73 74 00  f8 0d 79 0a 16 56 00 00  00 72 16 ba f2 10 00 00  00 00 00 00 00 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'device::FidoDeviceAuthenticator'
    #0 0x5616083d57b8  (/b/s/w/ir/out/Release/device_unittests+0x11b87b8)
    #1 0x561608fa1b34  (/b/s/w/ir/out/Release/device_unittests+0x1d84b34)
    #2 0x561608fa1f2f  (/b/s/w/ir/out/Release/device_unittests+0x1d84f2f)
    #3 0x561608815f60  (/b/s/w/ir/out/Release/device_unittests+0x15f8f60)
    #4 0x561608c82a31  (/b/s/w/ir/out/Release/device_unittests+0x1a65a31)
    #5 0x5616083c219d  (/b/s/w/ir/out/Release/device_unittests+0x11a519d)
    #6 0x561608a2bd65  (/b/s/w/ir/out/Release/device_unittests+0x180ed65)
    #7 0x561608a2d99d  (/b/s/w/ir/out/Release/device_unittests+0x181099d)
    #8 0x561608a2f302  (/b/s/w/ir/out/Release/device_unittests+0x1812302)
    #9 0x561608a47397  (/b/s/w/ir/out/Release/device_unittests+0x182a397)
    #10 0x561608a4629a  (/b/s/w/ir/out/Release/device_unittests+0x182929a)
    #11 0x561608c88138  (/b/s/w/ir/out/Release/device_unittests+0x1a6b138)
    #12 0x561608c8db42  (/b/s/w/ir/out/Release/device_unittests+0x1a70b42)
    #13 0x561608c8d97a  (/b/s/w/ir/out/Release/device_unittests+0x1a7097a)
    #14 0x5616084a0a3d  (/b/s/w/ir/out/Release/device_unittests+0x1283a3d)
    #15 0x7f1552137f44  (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)
    #16 0x561608013029  (/b/s/w/ir/out/Release/device_unittests+0xdf6029)



hongjunchoi, can you take a look?
 
Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
Status: Started (was: Assigned)
Thanks for reporting! I'll take a look. 
Components: Blink>WebAuthentication
Owner: martinkr@google.com
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 20

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

commit f2ecbbf87d916bb3f862010142fe81214bf5238c
Author: Martin Kreichgauer <martinkr@google.com>
Date: Tue Nov 20 00:47:58 2018

fido: fix a nonsensical cast in fido_request_handler_unittest

In crrev.com/c/1256016, we moved FidoAuthenticator instantiation and
ownership into the respective FidoDiscovery subclasses. This also
eliminated FidoRequestHandlerBase::CreateAuthenticatorFromDevice() which
was where we previously wrapped a discovered FidoDevice in a
FidoDeviceAuthenticator before adding it to the request handler.

In fido_request_handler_unittest, this was used as a testing seam to
inject a FakeFidoAuthenticator implementation. Injection of the fake was
dropped, but the "real" FidoDeviceAuthenticator was still casted into a
pointer to the fake elsewhere. While this seems to not bother compilers
too much, it's probably a Bad Idea. This CL therefore entirely removes
the FakeFidoAuthenticator class (without changing semantics of the
fixture or the tests).

Bug:  903294 
Change-Id: I7a6bad330b1152dc455ded5b6db3d3309f4eb3be
Reviewed-on: https://chromium-review.googlesource.com/c/1340903
Reviewed-by: Jun Choi <hongjunchoi@chromium.org>
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609536}
[modify] https://crrev.com/f2ecbbf87d916bb3f862010142fe81214bf5238c/device/fido/fido_device_authenticator.h
[modify] https://crrev.com/f2ecbbf87d916bb3f862010142fe81214bf5238c/device/fido/fido_request_handler_unittest.cc

Status: Fixed (was: Started)
The bot turned green, thanks for fixing! https://ci.chromium.org/buildbot/chromium.clang/ToTLinuxUBSanVptr/4617

Sign in to add a comment