Issue metadata
Sign in to add a comment
|
Use & invalidation of WeakPtr on different threads leads to racey "Null-dereference READ in chrome" |
||||||||||||||||||||||
Issue descriptionDetailed 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.
,
Jun 11 2018
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.
,
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.
,
Jun 12 2018
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.
,
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
,
Jun 14 2018
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by pnangunoori@chromium.org
, Jun 11 2018Labels: M-69 Test-Predator-Wrong
Owner: w...@chromium.org
Status: Assigned (was: Untriaged)