Merge "SF: Fix UAF on pacesetter change during commit" into main (d9da3e65) · Commits · e / os / platform_frameworks_native

services/surfaceflinger/Scheduler/Scheduler.cpp

+21 −21

Original line number	Diff line number	Diff line
		@@ -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);

		FrameTargeter& pacesetterTargeter = *pacesetterOpt->get().targeterPtr;
		pacesetterTargeter.beginFrame(beginFrameArgs, *pacesetterOpt->get().schedulePtr);
		ftl::NonNull<const Display*> pacesetterPtr = pacesetterPtrLocked();
		pacesetterPtr->targeterPtr->beginFrame(beginFrameArgs, *pacesetterPtr->schedulePtr);

		{
		FrameTargets targets;
		targets.try_emplace(pacesetterId, &pacesetterTargeter.target());
		targets.try_emplace(pacesetterPtr->displayId, &pacesetterPtr->targeterPtr->target());

		for (const auto& [id, display] : mDisplays) {
		if (id == pacesetterId) continue;
		if (id == pacesetterPtr->displayId) continue;

		FrameTargeter& targeter = *display.targeterPtr;
		targeter.beginFrame(beginFrameArgs, *display.schedulePtr);
		targets.try_emplace(id, &targeter.target());
		}

		if (!compositor.commit(pacesetterId, targets)) return;
		if (!compositor.commit(pacesetterPtr->displayId, 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) {

services/surfaceflinger/Scheduler/Scheduler.h

+6 −1

Original line number	Diff line number	Diff line
		@@ -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); })

services/surfaceflinger/tests/unittests/SchedulerTest.cpp

+58 −13

Original line number	Diff line number	Diff line
		@@ -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 {
		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 {