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

Issue 842484 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Regression
Team-Security-UX



Sign in to add a comment

Null-dereference READ in CommonNameMismatchHandler::OnSimpleLoaderResponseStarted

Project Member Reported by ClusterFuzz, May 13 2018

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=5672305049731072

Fuzzer: meacer_web_ext
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000010
Crash State:
  CommonNameMismatchHandler::OnSimpleLoaderResponseStarted
  base::internal::Invoker<base::internal::BindState<void
  network::SaveToStringBodyHandler::NotifyConsumerOfCompletion
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=557248:557251

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5672305049731072

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Cc: rsleevi@chromium.org brajkumar@chromium.org
Components: Internals>Network
Labels: -Type-Bug M-68 Test-Predator-Wrong Type-Bug-Regression
Owner: pilgrim@chromium.org
Status: Assigned (was: Untriaged)
Predator and CL could not provide any possible suspects.

Using Code Search for the file, "common_name_mismatch_handler.cc" suspecting the below Cl might have caused this issue

Suspect CL: https://chromium.googlesource.com/chromium/src/+/db972aaec73d8912a706c377d018a887a0547caf%5E%21/chrome/browser/ssl/common_name_mismatch_handler.cc

pilgrim@ -- Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Thanks!
Cc: mmenke@chromium.org
Components: -Internals>Network UI>Browser>Interstitials
pilgrim: Seems legit

mmenke: Adding for an extra set of eyes - does the SimpleURLLoader follow a different API contract than the URLFetcher regarding shutdown semantics and callbacks? This has the hallmark of using a callback after deletion, which, given the base::Unretained(), seems possible - but it'd also seem like we only got here on an errant PostTask, which the URLFetcher API seemed to avoid?
Labels: Proj-Servicification

Comment 4 by mmenke@chromium.org, May 14 2018

Deleting a SimpleURLLoader should cancel the request, and prevent all callbacks from being invoked.  Just looking at the code itself, I think the issue is most likely that response_head is nullptr, so CommonNameMismatchHandler::OnSimpleLoaderComplete() is passing in a nullptr as a const-ref.
D'oh, you're totally right. I was reading the 0x10 as the DCHECK(sequence_checker_), totally missing that the line number indicates it's when accessing response_head
on it
Project Member

Comment 7 by bugdroid1@chromium.org, May 14 2018

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

commit 7e6a1ff73ee461ce5809d1d2f80b307cbdcc163b
Author: Mark Pilgrim <pilgrim@chromium.org>
Date: Mon May 14 17:34:13 2018

Refactor CommonNameMismatchHandler to check null response head

Bug:  842484 
Test: CommonNameMismatchBrowserTest.NoCrashIfBothSubdomainsHaveCommonNameErrors
Change-Id: I6551dfbc129a544e449040c12a30236c6a57efe8
Reviewed-on: https://chromium-review.googlesource.com/1057417
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Commit-Queue: Mark Pilgrim <pilgrim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558365}
[modify] https://crrev.com/7e6a1ff73ee461ce5809d1d2f80b307cbdcc163b/chrome/browser/ssl/common_name_mismatch_handler.cc
[modify] https://crrev.com/7e6a1ff73ee461ce5809d1d2f80b307cbdcc163b/chrome/browser/ssl/common_name_mismatch_handler.h
[modify] https://crrev.com/7e6a1ff73ee461ce5809d1d2f80b307cbdcc163b/chrome/browser/ssl/ssl_browsertest.cc

Cc: pilgrim@chromium.org
 Issue 842855  has been merged into this issue.
Project Member

Comment 9 by ClusterFuzz, May 14 2018

Components: Internals>Core
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Components: -Internals>Core
Labels: Test-Predator-Wrong-Components
Project Member

Comment 11 by ClusterFuzz, May 15 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase 6537422192771072 is verified as fixed, so closing issue as verified.

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

Comment 12 by ClusterFuzz, May 15 2018

ClusterFuzz has detected this issue as fixed in range 558364:558366.

Detailed report: https://clusterfuzz.com/testcase?key=5672305049731072

Fuzzer: meacer_web_ext
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000010
Crash State:
  CommonNameMismatchHandler::OnSimpleLoaderResponseStarted
  base::internal::Invoker<base::internal::BindState<void
  network::SaveToStringBodyHandler::NotifyConsumerOfCompletion
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=557248:557251
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=558364:558366

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5672305049731072

See https://github.com/google/clusterfuzz-tools for more information.

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

Sign in to add a comment