New issue
Advanced search Search tips

Issue 609008 link

Starred by 0 users

Issue metadata

Status: WontFix
Owner: ----
Closed: Dec 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Crash in base::ThreadTaskRunnerHandle::Get

Project Member Reported by ClusterFuzz, May 4 2016

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5483006243897344

Fuzzer: inferno_twister_custom_bundle
Job Type: linux_asan_chrome_chromeos
Platform Id: linux

Crash Type: UNKNOWN READ
Crash Address: 0x000000000000
Crash State:
  base::ThreadTaskRunnerHandle::Get
  base::SequencedWorkerPool::SequencedWorkerPool
  base::LazyInstance<content::BrowserThreadGlobals, base::internal::LeakyLazyInsta
  

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv973BAPaAUs1PBbxbMNIGY4eTdR6Aasuy0e2OTF3kzGJ_LLhXBxVvrEF_WONZoUnLdwad1B2dpJxebbT2651C3YxoRNTcbdJEdUfdvgvHGA7_1OzP1HEeFgYT7M7XYSudRvhYe2CmlUq9S4SYoge-9HBb-ndiq4iUDCP8Pah60YYMnyCyGw


Additional requirements: Requires Gestures

Filer: brajkumar

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Labels: findit-wrong Te-Logged
Owner: gab@chromium.org
Status: Assigned (was: Available)
Through code search on file "thread_task_runner_handle.cc" from frame #1 suspecting the below change
Review URL: https://codereview.chromium.org/1911023002

Gab@ - Observed some recent changes on this file, so assigning to you. Could you please check if this is caused with respect to this change, if not please help us in reassign the issue to the right owner.

Thanks!

Comment 2 by gab@chromium.org, May 4 2016

Cc: gab@chromium.org
Owner: brajkumar@chromium.org
Okay so here's what's happening (and no it's not my CL):

base::ThreadTaskRunnerHandle::Get() assumes it's only called if base::ThreadTaskRunnerHandle::IsSet(), in this case it looks like it's not set and therefore Get() crashes on null deref (instead of hitting the DCHECK per this being a non-DCHECK build).

Why this happens:

base::SequencedWorkerPool's constructor assumes that 
base::ThreadTaskRunnerHandle::Get() is valid when it's being constructed (which is a reasonable assumption from where it's typically built).

In this stack though it's weirdly built as a result of a late Get() call to browser_thread_impl.cc's "LazyInstance<BrowserThreadGlobals> g_globals" during content::BrowserMainRunnerImpl::Shutdown() -> content::BrowserMainLoop::~BrowserMainLoop() -> etc.

It's surprising to me that g_globals didn't exist before that (i.e. it's not that it was deleted and re-created late as it's never deleted (LazyInstance::Leaky) but that is what's causing this.

So if there is a recent CL to blame it would be lower in that stack, whoever is causing the chain leading to this.

Lacking a better owner re-assigning for triage, sorry.
Labels: Needs-triage
Owner: ----
Status: Unconfirmed (was: Assigned)
Owner: skyos...@chromium.org
Status: Assigned (was: Unconfirmed)
From findit tool:

Author: skyostil
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src//+/9eb88f31c09e9fc61a6e3c654d2ab7c993d645ef
Time: Tue May 05 14:35:14 2015
The CL last changed line 1255 of file sequenced_worker_pool.cc, which is stack frame 2.

skyostil@, could you please take a look and help us to find correct owner if it is not your related changes.
Thank you.

Cc: skyos...@chromium.org
Owner: ----
Status: Available (was: Assigned)
My refactoring from a year ago probably wasn't the breaking change here :)

Here's the full stack trace. Maybe a ChromeOS thing?

    #0 0x7fd2b5128f03 in scoped_refptr base/memory/ref_counted.h:281:46
    #1 0x7fd2b5128f03 in base::ThreadTaskRunnerHandle::Get() base/thread_task_runner_handle.cc:28
    #2 0x7fd2b5132d23 in base::SequencedWorkerPool::SequencedWorkerPool(unsigned long, std::string const&) base/threading/sequenced_worker_pool.cc:1255:32
    #3 0x7fd2c149de40 in BrowserThreadGlobals content/browser/browser_thread_impl.cc:94:27
    #4 0x7fd2c149de40 in New base/lazy_instance.h:69
    #5 0x7fd2c149de40 in New base/lazy_instance.h:98
    #6 0x7fd2c149de40 in Pointer base/lazy_instance.h:163
    #7 0x7fd2c149de40 in base::LazyInstance<content::(anonymous namespace)::BrowserThreadGlobals, base::internal::LeakyLazyInstanceTraits<content::(anonymous namespace)::BrowserThreadGlobals> >::Get() base/lazy_instance.h:137
    #8 0x7fd2c14a1327 in content::BrowserThread::CurrentlyOn(content::BrowserThread::ID) content/browser/browser_thread_impl.cc:412:35
    #9 0x7fd2c4b9b476 in chromeos::BootTimesRecorder::AddMarker(std::vector<chromeos::BootTimesRecorder::TimeMarker, std::allocator<chromeos::BootTimesRecorder::TimeMarker> >*, chromeos::BootTimesRecorder::TimeMarker) chrome/browser/chromeos/boot_times_recorder.cc:465:7
    #10 0x7fd2c4b9bb8a in chromeos::BootTimesRecorder::AddLogoutTimeMarker(std::string const&, bool) chrome/browser/chromeos/boot_times_recorder.cc:456:3
    #11 0x7fd2c4bc1533 in chromeos::ChromeBrowserMainPartsChromeos::~ChromeBrowserMainPartsChromeos() chrome/browser/chromeos/chrome_browser_main_chromeos.cc:312:3
    #12 0x7fd2c4bc209a in chromeos::ChromeBrowserMainPartsChromeos::~ChromeBrowserMainPartsChromeos() chrome/browser/chromeos/chrome_browser_main_chromeos.cc:309:67
    #13 0x7fd2c1e6a239 in operator() /usr/include/c++/4.6/bits/unique_ptr.h:63:2
    #14 0x7fd2c1e6a239 in reset /usr/include/c++/4.6/bits/unique_ptr.h:245
    #15 0x7fd2c1e6a239 in ~unique_ptr /usr/include/c++/4.6/bits/unique_ptr.h:169
    #16 0x7fd2c1e6a239 in content::BrowserMainLoop::~BrowserMainLoop() content/browser/browser_main_loop.cc:454
    #17 0x7fd2c1e6ae0a in content::BrowserMainLoop::~BrowserMainLoop() content/browser/browser_main_loop.cc:450:37
    #18 0x7fd2c149adfb in operator() /usr/include/c++/4.6/bits/unique_ptr.h:63:2
    #19 0x7fd2c149adfb in reset /usr/include/c++/4.6/bits/unique_ptr.h:245
    #20 0x7fd2c149adfb in content::BrowserMainRunnerImpl::Shutdown() content/browser/browser_main_runner.cc:223
    #21 0x7fd2c14998bc in ~BrowserMainRunnerImpl content/browser/browser_main_runner.cc:59:7
    #22 0x7fd2c14998bc in content::BrowserMainRunnerImpl::~BrowserMainRunnerImpl() content/browser/browser_main_runner.cc:57
    #23 0x7fd2c1498e7a in operator() /usr/include/c++/4.6/bits/unique_ptr.h:63:2
    #24 0x7fd2c1498e7a in reset /usr/include/c++/4.6/bits/unique_ptr.h:245
    #25 0x7fd2c1498e7a in ~unique_ptr /usr/include/c++/4.6/bits/unique_ptr.h:169
    #26 0x7fd2c1498e7a in content::BrowserMain(content::MainFunctionParams const&) content/browser/browser_main.cc:51
    #27 0x7fd2b4ef46b3 in content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:742:12
    #28 0x7fd2b4ef044d in content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:20:15
    #29 0x7fd2b3ccc7e8 in ChromeMain chrome/app/chrome_main.cc:84:12
    #30 0x7fd2ab306ec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287
Project Member

Comment 6 by ClusterFuzz, Jun 2 2016

ClusterFuzz has detected this testcase as flaky and is unable to reproduce it in the original crash revision. Skipping fixed testing check and marking it as potentially fixed.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5483006243897344

Fuzzer: inferno_twister_custom_bundle
Job Type: linux_asan_chrome_chromeos
Platform Id: linux

Crash Type: UNKNOWN READ
Crash Address: 0x000000000000
Crash State:
  base::ThreadTaskRunnerHandle::Get
  base::SequencedWorkerPool::SequencedWorkerPool
  base::LazyInstance<content::BrowserThreadGlobals, base::internal::LeakyLazyInsta
  

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv973BAPaAUs1PBbxbMNIGY4eTdR6Aasuy0e2OTF3kzGJ_LLhXBxVvrEF_WONZoUnLdwad1B2dpJxebbT2651C3YxoRNTcbdJEdUfdvgvHGA7_1OzP1HEeFgYT7M7XYSudRvhYe2CmlUq9S4SYoge-9HBb-ndiq4iUDCP8Pah60YYMnyCyGw


Additional requirements: Requires Gestures

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 7 by ClusterFuzz, Jul 15 2016

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=4688130972319744

Fuzzer: libfuzzer_net_url_request_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: UNKNOWN READ
Crash Address: 0x000000000000
Crash State:
  scoped_refptr
  base::ThreadTaskRunnerHandle::Get
  GetTaskRunner
  

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv97GgUO5IyIF0rreWnpxViS8PirI82A-szXMYknXH5aoZ4wvRSnUkkrKdQyMvqDVNVkPgMOrXLZyXje-NbQog12EHm2KYue0Amq4GtGsMgfQEEONjalI_dTVSySga_icN_F_3G3aSWhEo4uJ7hkukeM_qOSTYQ?testcase_id=4688130972319744


Filer: mmohammad

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.
Cc: derat@chromium.org rbyers@chromium.org
Labels: -Needs-triage
Status: Untriaged (was: Available)
r124412 didn't quite do the trick.

Comment 9 by derat@chromium.org, Jul 18 2016

Cc: steve...@chromium.org
Labels: -OS-Linux OS-Chrome
I'm away from a shell right now, but maybe we just need to e.g. pass in another parameter to avoid calling BrowserThread::CurrentlyOn during shutdown:

// static
void BootTimesRecorder::AddMarker(std::vector<TimeMarker>* vector,
                                  TimeMarker marker) {
  // The marker vectors can only be safely manipulated on the main thread.
  // If we're late in the process of shutting down (eg. as can be the case at
  // logout), then we have to assume we're on the main thread already.
  if (BrowserThread::CurrentlyOn(BrowserThread::UI) ||
      !BrowserThread::IsMessageLoopValid(BrowserThread::UI)) {
    vector->push_back(marker);
  } else {
    // Add the marker on the UI thread.
    // Note that it's safe to use an unretained pointer to the vector because
    // BootTimesRecorder's lifetime exceeds that of the UI thread message loop.
    BrowserThread::PostTask(
        BrowserThread::UI, FROM_HERE,
        base::Bind(&BootTimesRecorder::AddMarker,
                   base::Unretained(vector),
                   marker));
  }
}
Labels: -ClusterFuzz Clusterfuzz
I don't know how to reproduce this, but would someone who does mind taking a look at my speculative fix at https://codereview.chromium.org/2200703002/ ?

I'm just switching the order of the !BrowserThread::IsMessageLoopValid() and BrowserThread::CurrentlyOn() in the hopes of short-circuiting the call to the latter in this case.
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 8 2016

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

commit 9972cd888b194adc1fc6de8bc6e53945846cb57e
Author: derat <derat@chromium.org>
Date: Mon Aug 08 20:28:57 2016

chromeos: Try to avoid shutdown crash in BootTimesRecorder.

Call BrowserThread::IsMessageLoopValid() (which bails out if
the BrowserThreadGlobals singleton is uninitialized) before
calling BrowserThread::CurrentlyOn() (which can apparently
deref a null pointer if base::ThreadTaskRunnerHandle hasn't
been initialized).

BUG= 609008 

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

[modify] https://crrev.com/9972cd888b194adc1fc6de8bc6e53945846cb57e/chrome/browser/chromeos/boot_times_recorder.cc

Project Member

Comment 12 by sheriffbot@chromium.org, Nov 22 2016

Labels: -Restrict-View-EditIssue
Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

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

Comment 13 by ClusterFuzz, Dec 22 2016

Status: WontFix (was: Untriaged)
ClusterFuzz testcase 4688130972319744 is flaky and no longer reproduces, so closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Sign in to add a comment