New issue
Advanced search Search tips

Issue 851378 link

Starred by 1 user

Issue metadata

Status: Duplicate
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Use & invalidation of WeakPtr on different threads leads to racey "Null-dereference READ in chrome"

Project Member Reported by ClusterFuzz, Jun 11 2018

Issue description

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

Fuzzer: phoglund_webrtc_peerconnection
Job Type: linux_cfi_chrome
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000088
Crash State:
  chrome
  base::internal::WeakReference::is_valid
  base::WeakPtr<content::PeerConnectionTracker>::get
  
Sanitizer: cfi (CFI)

Regressed: https://clusterfuzz.com/revisions?job=linux_cfi_chrome&range=565278:565318

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

Additional requirements: Requires HTTP

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Cc: pnangunoori@chromium.org
Labels: M-69 Test-Predator-Wrong
Owner: w...@chromium.org
Status: Assigned (was: Untriaged)
Predator and CL could not provide any possible suspects.
Using the code search for the file, “weak_ptr.cc” assigning to concern owner from GIT blame.

Suspecting Commit#
https://chromium.googlesource.com/chromium/src/+/7eedd9ec5a3d5c444289de752f7b01ccf30c22b5

@wez -- Could you please look into this issue, kindly reassign if it has nothing to do with your changes.
Thank You.

Comment 2 by w...@chromium.org, Jun 11 2018

Components: Blink>WebRTC
Labels: -Type-Bug Type-Bug-Regression
Owner: hbos@chromium.org
Summary: Use & invalidation of WeakPtr on different threads leads to racey "Null-dereference READ in chrome" (was: Null-dereference READ in chrome)
This is a null-page read at: https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/rtc_peer_connection_handler.cc?l=1950

  void RTCPeerConnectionHandler::OnSignalingChange(
      webrtc::PeerConnectionInterface::SignalingState new_state) {
    ...
    if (peer_connection_tracker_)
      peer_connection_tracker_->TrackSignalingStateChange(this, new_state);
    ...
  }

The only way this can happen is for the RTCPeerConnectionHandler |this| to be null - we're trying to use a member of a null object. :)

|this| originates from https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/rtc_peer_connection_handler.cc?l=957:

  void OnSuccessOnMainThread(
      webrtc::PeerConnectionInterface::SignalingState signaling_state) {
    ...
    if (handler_)
      handler_->OnSignalingChange(signaling_state);
    ...
  }

where |handler_| is itself another WeakPtr. The only way that |handler_| can be nullptr immediately after we checked it is if we're dereferencing |handler_| on a different thread to the one on which it is invalidated (e.g. by RTCPPeerConnectionHandler destruction - so we check it is not-null, the other thread happens to invalidate it, and then we dereference the now-invalid/null pointer.

It looks like hbos@ introduced this path in https://chromium-review.googlesource.com/1085302, but it's not immediately clear whether this is what introduced the threading bug; the RTCPeerConnectionHandler uses both a TaskRunner and ThreadChecker, and the use of WeakPtrs indicates that those are expected to refer to the same sequence, though that property is not explicitly enforce.
Project Member

Comment 3 by ClusterFuzz, Jun 12 2018

ClusterFuzz has detected this issue as fixed in range 565966:565984.

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

Fuzzer: phoglund_webrtc_peerconnection
Job Type: linux_cfi_chrome
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000088
Crash State:
  chrome
  base::internal::WeakReference::is_valid
  base::WeakPtr<content::PeerConnectionTracker>::get
  
Sanitizer: cfi (CFI)

Regressed: https://clusterfuzz.com/revisions?job=linux_cfi_chrome&range=565278:565318
Fixed: https://clusterfuzz.com/revisions?job=linux_cfi_chrome&range=565966:565984

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

Additional requirements: Requires HTTP

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.
Project Member

Comment 4 by ClusterFuzz, Jun 12 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase 5137437383458816 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 5 by bugdroid1@chromium.org, Jun 13 2018

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

commit cba6ea1f03591684c1e81086928e89356929be34
Author: Wez <wez@chromium.org>
Date: Wed Jun 13 06:51:50 2018

Use RunsTasksInCurrentSequence rather than ThreadChecker in handler.

Update RTCPeerConnectionHandler to check explicitly that the configured
|task_runner_| is the sequence that is is invoked on, rather than using
a separate ThreadChecker.

Bug:  851378 
Change-Id: Ib0e4a7f12e12c1e3b92a435244808d177f86cbf2
Reviewed-on: https://chromium-review.googlesource.com/1096277
Reviewed-by: Henrik Boström <hbos@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566739}
[modify] https://crrev.com/cba6ea1f03591684c1e81086928e89356929be34/content/renderer/media/webrtc/rtc_peer_connection_handler.cc
[modify] https://crrev.com/cba6ea1f03591684c1e81086928e89356929be34/content/renderer/media/webrtc/rtc_peer_connection_handler.h

Comment 6 by w...@chromium.org, Jun 14 2018

Mergedinto: 851369
Status: Duplicate (was: Verified)

Sign in to add a comment