SF: Fix UAF on pacesetter change during commit
During commit, the pacesetter's FrameTargeter could be destroyed after a
hotplug reconnect or a resolution change, via processDisplayChanged. The
reference in Scheduler::onFrameSignal was then dangling, causing a crash
when dereferenced later during composite.
Fixes: 308287117
Test: SchedulerTest.onFrameSignalMultipleDisplays
Change-Id: I413ee7d9967e731825106ef2b6d37fbfb15516ea
diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp
index 6437b4e..6610f94 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.cpp
+++ b/services/surfaceflinger/Scheduler/Scheduler.cpp
@@ -181,32 +181,33 @@
.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 @@
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 @@
}
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 0615b31..8a76436 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 @@
});
}
+ // 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 b5eb777..8a8f771 100644
--- a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
@@ -457,26 +457,51 @@
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 @@
}
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 {