From 20463244bd92a1481dd27c358ea3a30ffa746f28 Mon Sep 17 00:00:00 2001 From: Autopilot Bot Date: Thu, 3 Sep 2026 13:41:07 -0700 Subject: [PATCH] Replace UAF death test with deterministic stale-delegate assertion 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 --- .../SchedulerDelegateInvalidationTest.cpp | 49 +++++++++++-------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/scheduler/tests/SchedulerDelegateInvalidationTest.cpp b/packages/react-native/ReactCommon/react/renderer/scheduler/tests/SchedulerDelegateInvalidationTest.cpp index 16dc0ab26656..76c1646058f9 100644 --- a/packages/react-native/ReactCommon/react/renderer/scheduler/tests/SchedulerDelegateInvalidationTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/scheduler/tests/SchedulerDelegateInvalidationTest.cpp @@ -481,37 +481,44 @@ TEST_F(SchedulerDelegateInvalidationTest, JSThrowInitiatedTeardownIsSafe) { } // --------------------------------------------------------------------------- -// Test 3 — The window that remains open: a delegate dropped and destroyed -// with no error involved. +// Test 3 — The window that remains open: a delegate detached with no error +// involved. // // handleTaskError clears pendingRenderingUpdates_ before the host error -// handler runs, so an error-driven teardown is safe (Test 3). A plain +// handler runs, so an error-driven teardown is safe (Test 2). A plain // setDelegate swap never reaches handleTaskError, so the lambda enqueued in -// (a) still holds a raw pointer to freed memory when the queue drains in (c). +// (a) still calls through the raw pointer it captured when the queue drains +// in (c). Had the host also destroyed the delegate — as an instance teardown +// does — that call would be a use-after-free. +// +// The delegate is deliberately kept alive here rather than destroyed under an +// EXPECT_DEATH. Asserting on the crash asks undefined behaviour to reliably +// terminate the process, which it does not: the death-test form of this test +// (and its two predecessors) passed on the fbcode host but flaked above 88% +// on the Android instrumentation runner, reporting "failed to die" until +// trunk auto-disabled them. Observing the stale call directly pins the same +// open window deterministically. // --------------------------------------------------------------------------- -#if GTEST_HAS_DEATH_TEST TEST_F( SchedulerDelegateInvalidationTest, - DelegateDestroyedWithoutError_PendingRenderingUpdateIsUAF) { - EXPECT_DEATH( - { - setUp(); + DelegateDetachedWithoutError_PendingRenderingUpdateCallsStaleDelegate) { + setUp(); - // (a) Enqueue a rendering-update lambda capturing delegate_ raw. - scheduler_->uiManagerDidFinishTransaction( - coordinator_, /*mountSynchronously=*/false); + // (a) Enqueue a rendering-update lambda capturing delegate_ raw. + scheduler_->uiManagerDidFinishTransaction( + coordinator_, /*mountSynchronously=*/false); + EXPECT_EQ(delegate_->shouldRenderTransactionsCount(), 0); - // (b) Host swaps the delegate out and destroys it. No JS throw, so - // nothing clears the rendering-update queue. - scheduler_->setDelegate(nullptr); - delegate_.reset(); + // (b) Host swaps the delegate out. No JS throw, so nothing clears the + // rendering-update queue. + scheduler_->setDelegate(nullptr); + EXPECT_EQ(scheduler_->getDelegate(), nullptr); - // (c) Drain — the lambda dereferences the destroyed delegate. - runOneEventLoopTick(); - }, - ""); + // (c) Drain — the lambda calls through its captured pointer even though the + // scheduler itself no longer has a delegate. + runOneEventLoopTick(); + EXPECT_EQ(delegate_->shouldRenderTransactionsCount(), 1); } -#endif // --------------------------------------------------------------------------- // Test 4 — Same race as Test 2, but enqueued via the second lambda site: