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

Issue 765581 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Null-dereference READ in content::BlinkTestController::OnDumpFrameLayoutResponse

Project Member Reported by ClusterFuzz, Sep 15 2017

Issue description

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

Fuzzer: inferno_layout_test_unmodified
Job Type: mac_asan_content_shell
Platform Id: mac

Crash Type: Null-dereference READ
Crash Address: 0x000000000000
Crash State:
  content::BlinkTestController::OnDumpFrameLayoutResponse
  content::mojom::LayoutTestControl_DumpFrameLayout_ForwardToCallback::Accept
  mojo::InterfaceEndpointClient::HandleValidatedMessage
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=mac_asan_content_shell&range=482782:482851

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

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Cc: msrchandra@chromium.org kkaluri@chromium.org
Components: Infra>Client>Mojo Blink
Labels: M-62 Test-Predator-Wrong
Owner: lukasza@chromium.org
Status: Assigned (was: Untriaged)
Predator could not provide any possible suspects.
Using CL Search for the file, "blink_test_controller.cc" assigning to the concern owner who might be related or worked on similar file.

Suspected CL : https://chromium.googlesource.com/chromium/src/+/d565a833804f202c057a33337e63d55793144cec

lukasza@ -- Could you please look into the issue, kindly re-assign if this is not related to your changes.


Thank You.
Components: -Blink Blink>Layout
This is a duplicate of  issue 753647  (which ClusterFuzz has for some reason decided is fixed and verified) and very similar to  issue 735423  (and maybe  issue 760445 ).

I have no idea what is going on / how is the situation here possible.  But... to silence these bug reports, maybe I can put together a symptomatic fix and always bind a weak pointer instead of base::Unretained(this)?
WIP CL @ https://chromium-review.googlesource.com/677786
I am not proud...
Cc: palmer@chromium.org mbarbe...@chromium.org

Comment 6 by e...@chromium.org, Sep 28 2017

Components: -Blink>Layout Blink>Infra
Crash in blink test runner, updating component accordingly.
Project Member

Comment 7 by ClusterFuzz, Oct 1 2017

Components: Internals>Mojo Test
Labels: Test-Predator-AutoComponents
Automatically applying components based on information from OWNERS files. If this seems incorrect, please apply the Test-Predator-Wrong-Components label.
Components: -Internals>Mojo
Labels: Test-Predator-Wrong-Components
This doesn't seem like a core Mojo platform issue. Removing Internals>Mojo
Project Member

Comment 9 by ClusterFuzz, Oct 17 2017

Labels: OS-Linux
Labels: -Test-Predator-AutoComponents Test-Predator-Auto-Components
Project Member

Comment 11 by ClusterFuzz, Dec 13 2017

Labels: OS-Windows
Cc: pnangunoori@chromium.org lukasza@chromium.org
 Issue 781562  has been merged into this issue.
Project Member

Comment 13 by bugdroid1@chromium.org, Dec 19 2017

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

commit e679eb591b3d49ee08cacb83bf4b3e2d5cc4a80e
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Tue Dec 19 16:44:17 2017

Fix for UaF of |this| in BlinkTestController::OnDumpFrameLayoutResponse.

As shown in  https://crbug.com/765581 , it is possible that
BlinkTestController is destroyed before
BlinkTestController::OnDumpFrameLayoutResponse gets called:

1. To prevent UaF crashes in this case, this CL wraps |this| in a
   base::WeakPtr.  This is a speculative fix (I was never able to
   repro the UaF locally), but I am pretty confident it should avoid
   the crashes reported in  https://crbug.com/765581 

2. BlinkTestController should only be destroyed *after* a test is over.
   Therefore it is rather surprising that BlinkTestController gets
   destroyed while we are in the middle of generating a layout dump.
   To help demystify this surprise the CL adds a DCHECK to
   BlinkTestController::OnInitiateLayoutDump to make sure that
   only one layout dump is happening at a time.

Bug:  765581 
Change-Id: I07431bb60316d668c4bd1b465d25bcbfb12e6b96
Reviewed-on: https://chromium-review.googlesource.com/677786
Commit-Queue: Ɓukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525056}
[modify] https://crrev.com/e679eb591b3d49ee08cacb83bf4b3e2d5cc4a80e/content/shell/browser/layout_test/blink_test_controller.cc
[modify] https://crrev.com/e679eb591b3d49ee08cacb83bf4b3e2d5cc4a80e/content/shell/browser/layout_test/blink_test_controller.h

Status: Fixed (was: Assigned)
Marking as fixed - I am assumming that ClusterFuzz will complain if the issue still repros.
Project Member

Comment 15 by ClusterFuzz, Dec 26 2017

Labels: Needs-Feedback
ClusterFuzz testcase 4978265663209472 is still reproducing on tip-of-tree build (trunk).

Please re-test your fix against this testcase and if the fix was incorrect or incomplete, please re-open the bug. Otherwise, ignore this notification and add ClusterFuzz-Wrong label.

Sign in to add a comment