[webkit-changes] [WebKit/WebKit] a753a1: delayedRenderingUpdateDetectionTimer should hold a...

[Kiet Ho]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: a753a1d21c3368a763b9e9a50dca5e7d93f72f59
      https://github.com/WebKit/WebKit/commit/a753a1d21c3368a763b9e9a50dca5e7d93f72f59
  Author: Kiet Ho <tho22 at apple.com>
  Date:   2024-10-30 (Wed, 30 Oct 2024)

  Changed paths:
    M Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteLayerTreeEventDispatcher.cpp
    M Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteLayerTreeEventDispatcher.h

  Log Message:
  -----------
  delayedRenderingUpdateDetectionTimer should hold a weak pointer to the current RemoteLayerTreeEventDispatcher
https://bugs.webkit.org/show_bug.cgi?id=278943
rdar://133813795

Reviewed by Simon Fraser.

m_delayedRenderingUpdateDetectionTimer invokes
RemoteLayerTreeEventDispatcher::delayedRenderingUpdateDetectionTimerFired
on the current RemoteLayerTreeEventDispatcher object (`this`) when fired. However,
a race condition between when the timer is fired and when `this` is destroyed
can lead to a use-after-free:

1. RemoteLayerTreeEventDispatcherDisplayLinkClient::displayLinkFired is called
on the display link callback thread.
2. Previous method dispatch calls to RemoteLayerTreeEventDispatcher::didRefreshDisplay
in the scrolling thread. Once in the scrolling thread, it calls
RemoteLayerTreeEventDispatcher::scheduleDelayedRenderingUpdateDetectionTimer,
which schedules a one-shot timer to call
RemoteLayerTreeEventDispatcher::delayedRenderingUpdateDetectionTimerFired
within the context of `this`. The timer runs on the same thread as the
thread where it's scheduled - that is, the scrolling thread.
3. The timer is fired and RemoteLayerTreeEventDispatcher::delayedRenderingUpdateDetectionTimerFired
is called in the scrolling thread.
4. Just after the timer is fired and before the method accesses `this`, `this` is
destroyed in another thread.
5. In the scrolling thread, RemoteLayerTreeEventDispatcher::delayedRenderingUpdateDetectionTimerFired
executes without knowing `this` is destroyed. Eventually it accesses one of its
member and causes a UAF.

Fix this by making the timer function hold a weak pointer to `this`.
When fired, it checks if the weak pointer is still valid before using it.

Due to the race condition nature, the original fuzzer test case is flaky,
hence no tests.

* Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteLayerTreeEventDispatcher.cpp:
(WebKit::RemoteLayerTreeEventDispatcher::scheduleDelayedRenderingUpdateDetectionTimer):
Make the timer function hold a weak pointer to `this`.
* Source/WebKit/UIProcess/RemoteLayerTree/mac/RemoteLayerTreeEventDispatcher.h:

Originally-landed-as: 280938.303 at safari-7619-branch (c54b231c174f). rdar://138934262
Canonical link: https://commits.webkit.org/285942@main



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications