Skip to content

Commit d9ad3f0

Browse files
generatedunixname1608173377072046meta-codesync[bot]
authored andcommitted
Replace UAF death test with deterministic stale-delegate assertion (#58321)
Summary: Pull Request resolved: #58321 `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] Reviewed By: fkgozali Differential Revision: D118205636 fbshipit-source-id: 87cefd8db0d0ebf55025bb60fcdfd1e4cda46e35
1 parent 8cf8e09 commit d9ad3f0

1 file changed

Lines changed: 28 additions & 21 deletions

File tree

packages/react-native/ReactCommon/react/renderer/scheduler/tests/SchedulerDelegateInvalidationTest.cpp

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -481,37 +481,44 @@ TEST_F(SchedulerDelegateInvalidationTest, JSThrowInitiatedTeardownIsSafe) {
481481
}
482482

483483
// ---------------------------------------------------------------------------
484-
// Test 3 — The window that remains open: a delegate dropped and destroyed
485-
// with no error involved.
484+
// Test 3 — The window that remains open: a delegate detached with no error
485+
// involved.
486486
//
487487
// handleTaskError clears pendingRenderingUpdates_ before the host error
488-
// handler runs, so an error-driven teardown is safe (Test 3). A plain
488+
// handler runs, so an error-driven teardown is safe (Test 2). A plain
489489
// setDelegate swap never reaches handleTaskError, so the lambda enqueued in
490-
// (a) still holds a raw pointer to freed memory when the queue drains in (c).
490+
// (a) still calls through the raw pointer it captured when the queue drains
491+
// in (c). Had the host also destroyed the delegate — as an instance teardown
492+
// does — that call would be a use-after-free.
493+
//
494+
// The delegate is deliberately kept alive here rather than destroyed under an
495+
// EXPECT_DEATH. Asserting on the crash asks undefined behaviour to reliably
496+
// terminate the process, which it does not: the death-test form of this test
497+
// (and its two predecessors) passed on the fbcode host but flaked above 88%
498+
// on the Android instrumentation runner, reporting "failed to die" until
499+
// trunk auto-disabled them. Observing the stale call directly pins the same
500+
// open window deterministically.
491501
// ---------------------------------------------------------------------------
492-
#if GTEST_HAS_DEATH_TEST
493502
TEST_F(
494503
SchedulerDelegateInvalidationTest,
495-
DelegateDestroyedWithoutError_PendingRenderingUpdateIsUAF) {
496-
EXPECT_DEATH(
497-
{
498-
setUp();
504+
DelegateDetachedWithoutError_PendingRenderingUpdateCallsStaleDelegate) {
505+
setUp();
499506

500-
// (a) Enqueue a rendering-update lambda capturing delegate_ raw.
501-
scheduler_->uiManagerDidFinishTransaction(
502-
coordinator_, /*mountSynchronously=*/false);
507+
// (a) Enqueue a rendering-update lambda capturing delegate_ raw.
508+
scheduler_->uiManagerDidFinishTransaction(
509+
coordinator_, /*mountSynchronously=*/false);
510+
EXPECT_EQ(delegate_->shouldRenderTransactionsCount(), 0);
503511

504-
// (b) Host swaps the delegate out and destroys it. No JS throw, so
505-
// nothing clears the rendering-update queue.
506-
scheduler_->setDelegate(nullptr);
507-
delegate_.reset();
512+
// (b) Host swaps the delegate out. No JS throw, so nothing clears the
513+
// rendering-update queue.
514+
scheduler_->setDelegate(nullptr);
515+
EXPECT_EQ(scheduler_->getDelegate(), nullptr);
508516

509-
// (c) Drain — the lambda dereferences the destroyed delegate.
510-
runOneEventLoopTick();
511-
},
512-
"");
517+
// (c) Drain — the lambda calls through its captured pointer even though the
518+
// scheduler itself no longer has a delegate.
519+
runOneEventLoopTick();
520+
EXPECT_EQ(delegate_->shouldRenderTransactionsCount(), 1);
513521
}
514-
#endif
515522

516523
// ---------------------------------------------------------------------------
517524
// Test 4 — Same race as Test 2, but enqueued via the second lambda site:

0 commit comments

Comments
 (0)