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

Issue 607996 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Feb 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug


Sign in to add a comment

Fix existing UBSan vptr failures

Project Member Reported by krasin@chromium.org, Apr 29 2016

Issue description

Currently, a few tests suites are broken under ubsan vptr:
browser_tests components_unittests content_unittests ipc_tests net_unittests webkit_unit_tests

The UBSan vptr buildbots don't run these tests yet, and a cleanup is required to fix all the failures. This issue is to track this effort.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 29 2016

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

commit a6fd652c54e83981deb84d61fa2b7af8f9797a54
Author: krasin <krasin@google.com>
Date: Fri Apr 29 22:00:16 2016

UBSan vptr: disable broken test suites on the new GN buildbot.

They were never run on the GYP buildbot and a separate cleanup is required.

BUG= 607996 

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

[modify] https://crrev.com/a6fd652c54e83981deb84d61fa2b7af8f9797a54/testing/buildbot/chromium.fyi.json

Project Member

Comment 2 by bugdroid1@chromium.org, Apr 30 2016

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

commit 59fcd05345fcad7387085a745a6a254e0c70d6a3
Author: krasin <krasin@google.com>
Date: Sat Apr 30 18:57:51 2016

quic: check for stream_factory_ to be non-null before all accesses.

One of the missing checks was the reason for net_unittests to fail
under ubsan_vptr mode.

BUG= 607996 

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

[modify] https://crrev.com/59fcd05345fcad7387085a745a6a254e0c70d6a3/net/quic/quic_chromium_client_session.cc
[modify] https://crrev.com/59fcd05345fcad7387085a745a6a254e0c70d6a3/testing/buildbot/chromium.fyi.json

Comment 3 by krasin@chromium.org, May 17 2016

Blocking: 612594

Comment 4 by krasin@chromium.org, May 21 2016

As of now, the broken test suites are:

extensions_unittests, unit_tests, views_unittests.

Also, browser_tests components_unittests, content_unittests, ipc_tests, net_unittests, webkit_unit_tests are disabled by https://codereview.chromium.org/1933363002 and likely still broken as well.
Blockedon: 617199
Blockedon: 617206
Blockedon: 617305
Blockedon: 617332

Comment 9 by krasin@chromium.org, Sep 21 2016

Filed https://llvm.org/bugs/show_bug.cgi?id=30478 which is the reason for many webkit_unit_tests to fail under UBSan vptr.
For the completeness, here is the error message I see while running these tests:

$ ./out/ubsan-tot/webkit_unit_tests --gtest_filter=OnPaymentResponseTest.CanRequestEmail
IMPORTANT DEBUGGING NOTE: batches of tests are run inside their
own process. For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with
--single-process-tests.
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.
Note: Google Test filter = OnPaymentResponseTest.CanRequestEmail
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from OnPaymentResponseTest
[ RUN      ] OnPaymentResponseTest.CanRequestEmail
../../third_party/WebKit/Source/modules/payments/OnPaymentResponseTest.cpp:348:64: runtime error: member call on address 0x0b30c4763b98 which does not point to an object of type 'blink::mojom::blink::PaymentRequestClient'
0x0b30c4763b98: note: object is of type 'blink::PaymentRequest'
 00 00 00 00  58 35 7f 07 00 00 00 00  00 00 00 00 00 00 00 00  b8 3b 76 c4 30 0b 00 00  02 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'blink::PaymentRequest'
    #0 0x18b81b7 in blink::(anonymous namespace)::OnPaymentResponseTest_CanRequestEmail_Test::TestBody() third_party/WebKit/Source/modules/payments/OnPaymentResponseTest.cpp:348:64
    #1 0x2d64dd6 in testing::Test::Run() testing/gtest/src/gtest.cc:2474:5
    #2 0x2d661bd in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:11
    #3 0x2d67101 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:28
    #4 0x2d73ff7 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4647:43
    #5 0x2d731cd in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255:10
    #6 0x2cefb9f in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:46
    #7 0x2cefb9f in base::TestSuite::Run() base/test/test_suite.cc:246
    #8 0x7233a3 in (anonymous namespace)::runHelper(base::TestSuite*) third_party/WebKit/Source/web/tests/RunAllTests.cpp:50:29
    #9 0x2cf1b2b in Run base/callback.h:64:12
    #10 0x2cf1b2b in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&, int, int, bool, base::Callback<void (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) base/test/launcher/unit_test_launcher.cc:206
    #11 0x2cf19f5 in base::LaunchUnitTests(int, char**, base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) base/test/launcher/unit_test_launcher.cc:445:10
    #12 0x723287 in main third_party/WebKit/Source/web/tests/RunAllTests.cpp:74:12
    #13 0x7f2bfa28df44 in __libc_start_main /build/eglibc-oGUzwX/eglibc-2.19/csu/libc-start.c:287
    #14 0x5a7c0c in _start (/usr/local/google/home/krasin/chr22/src/out/ubsan-tot/webkit_unit_tests+0x5a7c0c)

[1/1] OnPaymentResponseTest.CanRequestEmail (CRASHED)
1 test crashed:
    OnPaymentResponseTest.CanRequestEmail (../../third_party/WebKit/Source/modules/payments/OnPaymentResponseTest.cpp:334)

The LLVM bug contains a minimal reproducer.
Bug listed in #9 is now fixed. With https://codereview.chromium.org/2455223002/ soon submitted, we'll have webkit_unit_tests running on UBSanVptr bots.
Blockedon: 660174
Looking at the list of problematic test suites in #4:

- webkit_unit_tests are fixed in https://codereview.chromium.org/2455223002/
- net_unittests are already running on UBSanVptr Linux; adding it to the ToT bot as a part of https://codereview.chromium.org/2455223002/
- ipc_tests are legitimately broken, filed  https://crbug.com/660174 

I have not looked into other suites yet.
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 28 2016

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

commit 623cf72475439f27bdcb556616887366b7566094
Author: krasin <krasin@chromium.org>
Date: Fri Oct 28 02:35:14 2016

Fix LayoutTableSectionTest test cases which called a method on nil.

In particular, section var was nil for the tests, which had
document().body()->firstChild()->firstChild()->layoutObject());

These are switched to getLayoutObjectByElementId("section"), and
also checks for section != NULL were added.

The bug was found with running the test with UBSan.

Also adding net_unittests to UBSanVptr ToT, as it has already been runnin on the regular UBSanVptr bot.

BUG= 607996 

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

[modify] https://crrev.com/623cf72475439f27bdcb556616887366b7566094/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/623cf72475439f27bdcb556616887366b7566094/third_party/WebKit/Source/core/layout/LayoutTableSectionTest.cpp

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 28 2016

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

commit 4e87d77ff1553c9e446009d330e128251d8d7d71
Author: krasin <krasin@chromium.org>
Date: Fri Oct 28 18:04:58 2016

Fix bad merge caused by repetitive nature of chromium.fyi.json.

This is a follow up to https://codereview.chromium.org/2455223002/

BUG= 607996 

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

[modify] https://crrev.com/4e87d77ff1553c9e446009d330e128251d8d7d71/testing/buildbot/chromium.fyi.json

Also, extensions_unittests and views_unittests pass at this moment. Modulo the fixes above, it's only unit_tests and browser_tests remaining broken.
Project Member

Comment 18 by bugdroid1@chromium.org, Oct 31 2016

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

commit 25c64861843f487feedf6eaa76acbec96187bb68
Author: krasin <krasin@chromium.org>
Date: Mon Oct 31 23:29:11 2016

Fix nullptr access in CursorRendererAura::SnapshotCursorState.

If active_window is nil, we should not call virtual methods on it.

The bug was found by running content_unittests under UBSan.

BUG= 607996 

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

[modify] https://crrev.com/25c64861843f487feedf6eaa76acbec96187bb68/content/browser/media/capture/cursor_renderer_aura.cc

unit_tests are green (locally)
browser_tests are broken:

NaClBrowserTest.*
PPAPINaCl.*
NewlibPackagedAppTest.*
ChromeServiceWorkerFetchPPAPI.*
ChromeNavigationPortMappedBrowserTest.ContextMenuNavigationToInvalidUrl
CrSettingsMainPageTest.All
MaterialHistoryBrowserTest.*
MediaGalleriesPlatformAppPpapiTest.SendFilesystem
MediaRouterElementsBrowserTest.MediaRouterContainerSearch
PrintPreviewWebUITest.TestAdvancedSettings1Option
PrintPreviewWebUITest.TestAdvancedSettings2Options
SettingsAutofillSectionBrowserTest.AddressLocaleTests
SettingsPasswordSectionBrowserTest.uiTests 

At least some of the tests are failing due to  https://crbug.com/520760 .
Project Member

Comment 20 by bugdroid1@chromium.org, Nov 1 2016

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

commit dd5416a6158105826292b69875c214464000d7b3
Author: krasin <krasin@chromium.org>
Date: Tue Nov 01 17:02:37 2016

Initialize renderer ppapi host in ppapi_unittests to fix nullptr access.

Otherwise, PepperPluginInstanceImpl will call
host_impl->CreateInProcessResourceCreationAPI(this) on nullptr.

The bug was found by running the tests under UBSan.

BUG= 607996 

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

[modify] https://crrev.com/dd5416a6158105826292b69875c214464000d7b3/content/renderer/pepper/renderer_ppapi_host_impl.h
[modify] https://crrev.com/dd5416a6158105826292b69875c214464000d7b3/content/test/ppapi_unittest.cc

Project Member

Comment 21 by bugdroid1@chromium.org, Nov 2 2016

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

commit ebf0e626c43b986e5dfed5e961575bf31760a11c
Author: krasin <krasin@chromium.org>
Date: Wed Nov 02 21:44:58 2016

Fix vcall on nullptr in ~Performance.

This is a follow up to https://codereview.chromium.org/2449673002/

BUG= 607996 

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

[modify] https://crrev.com/ebf0e626c43b986e5dfed5e961575bf31760a11c/third_party/WebKit/Source/core/timing/Performance.cpp

Project Member

Comment 22 by bugdroid1@chromium.org, Nov 2 2016

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

commit 227fb8caff62e3049fc45e238bc1d6b9fc336463
Author: krasin <krasin@chromium.org>
Date: Wed Nov 02 22:45:48 2016

Add more tests to run on UBSan bots, as they are cleaned of existing failures.

BUG= 607996 

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

[modify] https://crrev.com/227fb8caff62e3049fc45e238bc1d6b9fc336463/testing/buildbot/chromium.fyi.json

Project Member

Comment 23 by bugdroid1@chromium.org, Nov 3 2016

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

commit 7bacd498506b1089ec53cddf76152baa4840bc24
Author: krasin <krasin@chromium.org>
Date: Thu Nov 03 21:12:50 2016

Blacklist nacl_message_scanner for UBSan.

The code is doing unsafe (and potentially wrong, if the compiler comes to the play)
casts, which UBSan and CFI are not happy about. CFI already has this
file blacklisted, it's now the time for UBSan to join the party.

BUG= 520760 , 607996 

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

[modify] https://crrev.com/7bacd498506b1089ec53cddf76152baa4840bc24/tools/ubsan/vptr_blacklist.txt

I have added nacl_message_scanner.cc into a blacklist (it's there for CFI already):
https://codereview.chromium.org/2476733002/

That makes the following test cases in browser_tests green:

NaClBrowserTest*
PPAPINaCl*
NewlibPackagedAppTest*
ChromeServiceWorkerFetchPPAPI*
MediaGalleriesPlatformAppPpapiTest.SendFilesystem

Timed out:
ChromeNavigationPortMappedBrowserTest.ContextMenuNavigationToInvalidUrl

Failed (all with the same stack trace)
CrSettingsMainPageTest.All
MediaRouterElementsBrowserTest.MediaRouterContainerSearch (same stack trace as above)
MaterialHistoryBrowserTest* (same stack trace as above)
PrintPreviewWebUITest.TestAdvancedSettings1Option
PrintPreviewWebUITest.TestAdvancedSettings2Options
SettingsAutofillSectionBrowserTest.AddressLocaleTests
SettingsPasswordSectionBrowserTest.uiTests 

The stack trace:
Received signal 11 SEGV_MAPERR 000000000000
#0 0x0000049efa66 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#1 0x7ffff4e0b330 <unknown>
#2 0x00000bb72b00 blink::CompositedLayerMapping::containingSquashedLayer()
#3 0x00000bbcdb66 blink::CompositingLayerAssigner::getReasonsPreventingSquashing()
#4 0x00000bbcb77d blink::CompositingLayerAssigner::assignLayersToBackingsInternal()
#5 0x00000bbcc571 blink::CompositingLayerAssigner::assignLayersToBackingsInternal()
#6 0x00000bbcc571 blink::CompositingLayerAssigner::assignLayersToBackingsInternal()
#7 0x00000bbcb4ac blink::CompositingLayerAssigner::assign()
#8 0x00000bb84ddf blink::PaintLayerCompositor::updateIfNeeded()
#9 0x00000bb83ff7 blink::PaintLayerCompositor::updateIfNeededRecursiveInternal()
#10 0x00000bb835d7 blink::PaintLayerCompositor::updateIfNeededRecursive()
#11 0x00000b2a004d blink::FrameView::updateLifecyclePhasesInternal()
#12 0x00000bd49bd6 blink::PageAnimator::updateAllLifecyclePhases()
#13 0x00000a630e05 blink::WebViewImpl::updateAllLifecyclePhases()
#14 0x00000ddeaa8a cc::ProxyMain::BeginMainFrame()
#15 0x00000de0735f _ZN4base8internal13FunctorTraitsIMN2cc9ProxyMainEFvSt10unique_ptrINS2_28BeginMainFrameAndCommitStateESt14default_deleteIS5_EEEvE6InvokeIRKNS_7WeakPtrIS3_EEJS8_EEEvSA_OT_DpOT0_
#16 0x00000de07267 _ZN4base8internal7InvokerINS0_9BindStateIMN2cc9ProxyMainEFvSt10unique_ptrINS3_28BeginMainFrameAndCommitStateESt14default_deleteIS6_EEEJNS_7WeakPtrIS4_EENS0_13PassedWrapperIS9_EEEEEFvvEE7RunIm
plIRKSB_RKSt5tupleIJSD_SF_EEJLm0ELm1EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#17 0x000004acd82b base::debug::TaskAnnotator::RunTask()
#18 0x00000a50e74b blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue()
#19 0x00000a50a84a blink::scheduler::TaskQueueManager::DoWork()
#20 0x000004acd82b base::debug::TaskAnnotator::RunTask()
#21 0x000004a15e30 base::MessageLoop::RunTask()
#22 0x000004a162d9 base::MessageLoop::DeferOrRunPendingTask()
#23 0x000004a16c13 base::MessageLoop::DoWork()
#24 0x000004a1a033 base::MessagePumpDefault::Run()
#25 0x000004a156ed base::MessageLoop::RunHandler()
#26 0x000004a55c89 base::RunLoop::Run()
#27 0x00000cc6c4dc content::RendererMain()
#28 0x0000049c258b content::RunZygote()
#29 0x0000049c4d0b content::ContentMainRunnerImpl::Run()
#30 0x0000049c1ee7 content::ContentMain()
#31 0x000005964843 content::LaunchTests()

That looks like a call on a nil object. Will take a closer look later.
Blockedon: 662265
One more fix for a problem that only pops up with dcheck is enabled (and the current UBSanVptr Linux bot does not have it enabled): https://codereview.chromium.org/2474863003/
Project Member

Comment 27 by bugdroid1@chromium.org, Nov 4 2016

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

commit 56c5f7e3a885b62cc821f1847a80f1652a70da2f
Author: krasin <krasin@chromium.org>
Date: Fri Nov 04 04:28:47 2016

Disable DisplayItemClientTest.IsAlive under UBSan.

This test makes a virtual call on a bad pointer, and
UBSan is rightfully not happy about it.

BUG= 607996 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[modify] https://crrev.com/56c5f7e3a885b62cc821f1847a80f1652a70da2f/third_party/WebKit/Source/platform/graphics/paint/DisplayItemClientTest.cpp

Current failures in browser_tests (as predicted by the trybot):
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_ubsan_rel_ng/builds/5

ComponentCloudPolicyTest.InstallNewExtension
DeviceManagementServiceIntegrationTestInstance/DeviceManagementServiceIntegrationTest.Unregistration/1
DeviceManagementServiceIntegrationTestInstance/DeviceManagementServiceIntegrationTest.Registration/1
CloudPolicyTest.FetchPolicy
ComponentCloudPolicyTest.FetchExtensionPolicy
ComponentCloudPolicyTest.UpdateExtensionPolicy
DeviceManagementServiceIntegrationTestInstance/DeviceManagementServiceIntegrationTest.PolicyFetch/1
DeviceManagementServiceIntegrationTestInstance/DeviceManagementServiceIntegrationTest.ApiAuthCodeFetch/1
CloudPolicyTest.InvalidatePolicy
ComponentCloudPolicyTest.SignOutAndBackIn
DeviceManagementServiceIntegrationTestInstance/DeviceManagementServiceIntegrationTest.AutoEnrollment/1

Blockedon: 663552
Status: Fixed (was: Untriaged)
The trybot is green: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_ubsan_rel_ng

Sign in to add a comment