Merge "SF: Fix UAF on pacesetter change during commit" into main

3 files changed, 85 insertions, 35 deletions

diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp
index 6437b4eb94..6610f948a7 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.cpp
+++ b/services/surfaceflinger/Scheduler/Scheduler.cpp
@@ -181,32 +181,33 @@ void Scheduler::onFrameSignal(ICompositor& compositor, VsyncId vsyncId,
              .expectedVsyncTime = expectedVsyncTime,
              .sfWorkDuration = mVsyncModulator->getVsyncConfig().sfWorkDuration};
 
-    LOG_ALWAYS_FATAL_IF(!mPacesetterDisplayId);
-    const auto pacesetterId = *mPacesetterDisplayId;
-    const auto pacesetterOpt = mDisplays.get(pacesetterId);
+    ftl::NonNull<const Display*> pacesetterPtr = pacesetterPtrLocked();
+    pacesetterPtr->targeterPtr->beginFrame(beginFrameArgs, *pacesetterPtr->schedulePtr);
 
-    FrameTargeter& pacesetterTargeter = *pacesetterOpt->get().targeterPtr;
-    pacesetterTargeter.beginFrame(beginFrameArgs, *pacesetterOpt->get().schedulePtr);
+    {
+        FrameTargets targets;
+        targets.try_emplace(pacesetterPtr->displayId, &pacesetterPtr->targeterPtr->target());
 
-    FrameTargets targets;
-    targets.try_emplace(pacesetterId, &pacesetterTargeter.target());
+        for (const auto& [id, display] : mDisplays) {
+            if (id == pacesetterPtr->displayId) continue;
 
-    for (const auto& [id, display] : mDisplays) {
-        if (id == pacesetterId) continue;
+            FrameTargeter& targeter = *display.targeterPtr;
+            targeter.beginFrame(beginFrameArgs, *display.schedulePtr);
+            targets.try_emplace(id, &targeter.target());
+        }
 
-        FrameTargeter& targeter = *display.targeterPtr;
-        targeter.beginFrame(beginFrameArgs, *display.schedulePtr);
-        targets.try_emplace(id, &targeter.target());
+        if (!compositor.commit(pacesetterPtr->displayId, targets)) return;
     }
 
-    if (!compositor.commit(pacesetterId, targets)) return;
+    // The pacesetter may have changed or been registered anew during commit.
+    pacesetterPtr = pacesetterPtrLocked();
 
     // TODO(b/256196556): Choose the frontrunner display.
     FrameTargeters targeters;
-    targeters.try_emplace(pacesetterId, &pacesetterTargeter);
+    targeters.try_emplace(pacesetterPtr->displayId, pacesetterPtr->targeterPtr.get());
 
     for (auto& [id, display] : mDisplays) {
-        if (id == pacesetterId) continue;
+        if (id == pacesetterPtr->displayId) continue;
 
         FrameTargeter& targeter = *display.targeterPtr;
         targeters.try_emplace(id, &targeter);
@@ -214,7 +215,7 @@ void Scheduler::onFrameSignal(ICompositor& compositor, VsyncId vsyncId,
 
     if (FlagManager::getInstance().vrr_config() &&
         CC_UNLIKELY(mPacesetterFrameDurationFractionToSkip > 0.f)) {
-        const auto period = pacesetterTargeter.target().expectedFrameDuration();
+        const auto period = pacesetterPtr->targeterPtr->target().expectedFrameDuration();
         const auto skipDuration = Duration::fromNs(
                 static_cast<nsecs_t>(period.ns() * mPacesetterFrameDurationFractionToSkip));
         ATRACE_FORMAT("Injecting jank for %f%% of the frame (%" PRId64 " ns)",
@@ -224,19 +225,18 @@ void Scheduler::onFrameSignal(ICompositor& compositor, VsyncId vsyncId,
     }
 
     if (FlagManager::getInstance().vrr_config()) {
-        const auto minFramePeriod = pacesetterOpt->get().schedulePtr->minFramePeriod();
+        const auto minFramePeriod = pacesetterPtr->schedulePtr->minFramePeriod();
         const auto presentFenceForPastVsync =
-                pacesetterTargeter.target().presentFenceForPastVsync(minFramePeriod);
+                pacesetterPtr->targeterPtr->target().presentFenceForPastVsync(minFramePeriod);
         const auto lastConfirmedPresentTime = presentFenceForPastVsync->getSignalTime();
         if (lastConfirmedPresentTime != Fence::SIGNAL_TIME_PENDING &&
             lastConfirmedPresentTime != Fence::SIGNAL_TIME_INVALID) {
-            pacesetterOpt->get()
-                    .schedulePtr->getTracker()
+            pacesetterPtr->schedulePtr->getTracker()
                     .onFrameBegin(expectedVsyncTime, TimePoint::fromNs(lastConfirmedPresentTime));
         }
     }
 
-    const auto resultsPerDisplay = compositor.composite(pacesetterId, targeters);
+    const auto resultsPerDisplay = compositor.composite(pacesetterPtr->displayId, targeters);
     compositor.sample();
 
     for (const auto& [id, targeter] : targeters) {
diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h
index 0615b31c10..8a76436fda 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.h
+++ b/services/surfaceflinger/Scheduler/Scheduler.h
@@ -33,6 +33,7 @@
 #pragma clang diagnostic pop // ignored "-Wconversion -Wextra"
 
 #include <ftl/fake_guard.h>
+#include <ftl/non_null.h>
 #include <ftl/optional.h>
 #include <scheduler/Features.h>
 #include <scheduler/FrameTargeter.h>
@@ -516,13 +517,17 @@ private:
                                                      });
     }
 
+    // The pacesetter must exist as a precondition.
+    ftl::NonNull<const Display*> pacesetterPtrLocked() const REQUIRES(mDisplayLock) {
+        return ftl::as_non_null(&pacesetterDisplayLocked()->get());
+    }
+
     RefreshRateSelectorPtr pacesetterSelectorPtr() const EXCLUDES(mDisplayLock) {
         std::scoped_lock lock(mDisplayLock);
         return pacesetterSelectorPtrLocked();
     }
 
     RefreshRateSelectorPtr pacesetterSelectorPtrLocked() const REQUIRES(mDisplayLock) {
-        ftl::FakeGuard guard(kMainThreadContext);
         return pacesetterDisplayLocked()
                 .transform([](const Display& display) { return display.selectorPtr; })
                 .or_else([] { return std::optional<RefreshRateSelectorPtr>(nullptr); })
diff --git a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
index b5eb777a4d..8a8f771a1d 100644
--- a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
@@ -457,26 +457,51 @@ TEST_F(SchedulerTest, onFrameSignalMultipleDisplays) {
     using VsyncIds = std::vector<std::pair<PhysicalDisplayId, VsyncId>>;
 
     struct Compositor final : ICompositor {
-        VsyncIds vsyncIds;
+        explicit Compositor(TestableScheduler& scheduler) : scheduler(scheduler) {}
+
+        TestableScheduler& scheduler;
+
+        struct {
+            PhysicalDisplayId commit;
+            PhysicalDisplayId composite;
+        } pacesetterIds;
+
+        struct {
+            VsyncIds commit;
+            VsyncIds composite;
+        } vsyncIds;
+
         bool committed = true;
+        bool changePacesetter = false;
 
         void configure() override {}
 
-        bool commit(PhysicalDisplayId, const scheduler::FrameTargets& targets) override {
-            vsyncIds.clear();
+        bool commit(PhysicalDisplayId pacesetterId,
+                    const scheduler::FrameTargets& targets) override {
+            pacesetterIds.commit = pacesetterId;
+
+            vsyncIds.commit.clear();
+            vsyncIds.composite.clear();
 
             for (const auto& [id, target] : targets) {
-                vsyncIds.emplace_back(id, target->vsyncId());
+                vsyncIds.commit.emplace_back(id, target->vsyncId());
+            }
+
+            if (changePacesetter) {
+                scheduler.setPacesetterDisplay(kDisplayId2);
             }
 
             return committed;
         }
 
-        CompositeResultsPerDisplay composite(PhysicalDisplayId,
-                                             const scheduler::FrameTargeters&) override {
+        CompositeResultsPerDisplay composite(PhysicalDisplayId pacesetterId,
+                                             const scheduler::FrameTargeters& targeters) override {
+            pacesetterIds.composite = pacesetterId;
+
             CompositeResultsPerDisplay results;
 
-            for (const auto& [id, _] : vsyncIds) {
+            for (const auto& [id, targeter] : targeters) {
+                vsyncIds.composite.emplace_back(id, targeter->target().vsyncId());
                 results.try_emplace(id,
                                     CompositeResult{.compositionCoverage =
                                                             CompositionCoverage::Hwc});
@@ -486,21 +511,41 @@ TEST_F(SchedulerTest, onFrameSignalMultipleDisplays) {
         }
 
         void sample() override {}
-    } compositor;
+    } compositor(*mScheduler);
 
     mScheduler->doFrameSignal(compositor, VsyncId(42));
 
-    const auto makeVsyncIds = [](VsyncId vsyncId) -> VsyncIds {
-        return {{kDisplayId1, vsyncId}, {kDisplayId2, vsyncId}};
+    const auto makeVsyncIds = [](VsyncId vsyncId, bool swap = false) -> VsyncIds {
+        if (swap) {
+            return {{kDisplayId2, vsyncId}, {kDisplayId1, vsyncId}};
+        } else {
+            return {{kDisplayId1, vsyncId}, {kDisplayId2, vsyncId}};
+        }
     };
 
-    EXPECT_EQ(makeVsyncIds(VsyncId(42)), compositor.vsyncIds);
+    EXPECT_EQ(kDisplayId1, compositor.pacesetterIds.commit);
+    EXPECT_EQ(kDisplayId1, compositor.pacesetterIds.composite);
+    EXPECT_EQ(makeVsyncIds(VsyncId(42)), compositor.vsyncIds.commit);
+    EXPECT_EQ(makeVsyncIds(VsyncId(42)), compositor.vsyncIds.composite);
 
+    // FrameTargets should be updated despite the skipped commit.
     compositor.committed = false;
     mScheduler->doFrameSignal(compositor, VsyncId(43));
 
-    // FrameTargets should be updated despite the skipped commit.
-    EXPECT_EQ(makeVsyncIds(VsyncId(43)), compositor.vsyncIds);
+    EXPECT_EQ(kDisplayId1, compositor.pacesetterIds.commit);
+    EXPECT_EQ(kDisplayId1, compositor.pacesetterIds.composite);
+    EXPECT_EQ(makeVsyncIds(VsyncId(43)), compositor.vsyncIds.commit);
+    EXPECT_TRUE(compositor.vsyncIds.composite.empty());
+
+    // The pacesetter may change during commit.
+    compositor.committed = true;
+    compositor.changePacesetter = true;
+    mScheduler->doFrameSignal(compositor, VsyncId(44));
+
+    EXPECT_EQ(kDisplayId1, compositor.pacesetterIds.commit);
+    EXPECT_EQ(kDisplayId2, compositor.pacesetterIds.composite);
+    EXPECT_EQ(makeVsyncIds(VsyncId(44)), compositor.vsyncIds.commit);
+    EXPECT_EQ(makeVsyncIds(VsyncId(44), true), compositor.vsyncIds.composite);
 }
 
 class AttachedChoreographerTest : public SchedulerTest {