Replace UAF death test with deterministic stale-delegate assertion - #58321
Closed
fkgozali wants to merge 1 commit into
Closed
Replace UAF death test with deterministic stale-delegate assertion#58321fkgozali wants to merge 1 commit into
fkgozali wants to merge 1 commit into
Conversation
Summary: `SchedulerDelegateInvalidationTest.DelegateDestroyedWithoutError_PendingRenderingUpdateIsUAF` asserted a real use-after-free through `EXPECT_DEATH`: it destroyed the `RecordingDelegate`, then drained `pendingRenderingUpdates_` so the queued lambda dereferenced the freed object, and expected the process to die. Undefined behaviour is not a reliable process-termination signal, without a sanitizer the freed read can simply succeed, and gtest then reports `Result: failed to die.` The death test also has to `fork()` a multi-threaded process. This replaces the death test with a deterministic assertion on the same property. Instead of destroying the delegate, the test detaches it via `Scheduler::setDelegate(nullptr)` and keeps it alive, then drains the pending rendering update and asserts that the drained lambda still invokes `schedulerShouldRenderTransactions` on the detached delegate. That pins exactly the coverage the death test was after: `Scheduler::setDelegate` is a plain assignment, so a lambda already queued by `uiManagerDidFinishTransaction` keeps the raw delegate pointer it captured, and draining it after the delegate has been detached still calls through that pointer, which is a use-after-free when the delegate has been destroyed rather than merely detached. Same property, no undefined behaviour and no `fork()`. It matches the shape of the existing `UnregisterSurface_DoesNotDrainPendingRenderingUpdates` test in the same file. The underlying window is unchanged: nothing in `Scheduler::setDelegate` cancels rendering updates that are already queued, so closing it needs a shutdown signal at the runtime-scheduler level. That is a design decision for the owners rather than a test fix. Changelog: [Internal] Differential Revision: D118205636
|
@fkgozali has exported this pull request. If you are a Meta employee, you can view the originating Diff in D118205636. |
cortinico
approved these changes
Sep 3, 2026
cortinico
left a comment
Contributor
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
|
This pull request has been merged in d9ad3f0. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
SchedulerDelegateInvalidationTest.DelegateDestroyedWithoutError_PendingRenderingUpdateIsUAFasserted a real use-after-free through
EXPECT_DEATH: it destroyed theRecordingDelegate, then drainedpendingRenderingUpdates_so the queued lambdadereferenced the freed object, and expected the process to die. Undefined behaviour
is not a reliable process-termination signal, without a sanitizer the freed read
can simply succeed, and gtest then reports
Result: failed to die.The death testalso has to
fork()a multi-threaded process.This replaces the death test with a deterministic assertion on the same property.
Instead of destroying the delegate, the test detaches it via
Scheduler::setDelegate(nullptr)and keeps it alive, then drains the pendingrendering update and asserts that the drained lambda still invokes
schedulerShouldRenderTransactionson the detached delegate.That pins exactly the coverage the death test was after:
Scheduler::setDelegateisa plain assignment, so a lambda already queued by
uiManagerDidFinishTransactionkeeps the raw delegate pointer it captured, and draining it after the delegate has
been detached still calls through that pointer, which is a use-after-free when the
delegate has been destroyed rather than merely detached. Same property, no undefined
behaviour and no
fork(). It matches the shape of the existingUnregisterSurface_DoesNotDrainPendingRenderingUpdatestest in the same file.The underlying window is unchanged: nothing in
Scheduler::setDelegatecancelsrendering updates that are already queued, so closing it needs a shutdown signal at
the runtime-scheduler level. That is a design decision for the owners rather than a
test fix.
Changelog: [Internal]
Differential Revision: D118205636