Merge "end2end: Fix UB in refresh event time computations [10/N]" into main (ffb4135b) · Commits · e / os / android_frameworks_native

services/surfaceflinger/tests/end2end/test_framework/hwc3/SingleDisplayRefreshSchedule.cpp

+123 −14

Original line number	Diff line number	Diff line
		@@ -14,9 +14,13 @@
		* limitations under the License.
		*/

		#include <bit>
		#include <chrono>
		#include <cstdint>
		#include <limits>
		#include <string>

		#include <android-base/logging.h>
		#include <fmt/chrono.h> // NOLINT(misc-include-cleaner)
		#include <fmt/format.h>

		@@ -24,21 +28,126 @@

		namespace android::surfaceflinger::tests::end2end::test_framework::hwc3 {

		namespace {

		using TimePoint = std::chrono::steady_clock::time_point;

		// std::chrono::steady_clock uses signed 64 bit values for time points, but that complicates
		// this computation. We offset those by this amount so TimePoint::min() becomes zero, and
		// TimePoint::max() becomes UINT64_MAX.
		constexpr auto kSignedToUnsignedOffset = std::numeric_limits<uint64_t>::max() / 2 + 1;

		// Converts a TimePoint to a bare uint64_t count.
		[[nodiscard]] constexpr auto timePointToUnsigned(TimePoint value) -> uint64_t {
		return std::bit_cast<uint64_t>(value.time_since_epoch().count()) + kSignedToUnsignedOffset;
		};

		// Converts a bare uint64_t count to a TimePoint.
		[[nodiscard]] constexpr auto timePointFromUnsigned(uint64_t value) -> TimePoint {
		return TimePoint{TimePoint::duration(std::bit_cast<int64_t>(value - kSignedToUnsignedOffset))};
		};

		// Computes the next event time after the input (forTimeNs), given the period and phase offset for
		// the event schedule.
		[[nodiscard]] constexpr auto computeNextEventTime(uint64_t periodNs, uint64_t phaseOffsetNs,
		uint64_t forTimeNs) -> uint64_t {
		// By adding or not adding periodNs, we can eliminate underflow and overflow in the modulus
		// portion of the computation, as doing so does not change the post-modulus result.
		const uint64_t avoidUnderAndOverflow = (forTimeNs < periodNs) ? periodNs : 0;

		// Note that adding periodNs here overflows if the next event time is greater than UINT64_MAX.
		return forTimeNs + periodNs - ((forTimeNs + avoidUnderAndOverflow - phaseOffsetNs) % periodNs);
		}

		// NOLINTBEGIN(*-magic-numbers)

		// These compile time checks ensure the computations are free of undefined behavior.

		// Ensure timePointToUnsigned maps TimePoint::min() to zero
		static_assert(timePointToUnsigned(TimePoint::min()) == 0);

		// Ensure timePointToUnsigned maps TimePoint::max() to UINT64_MAX
		static_assert(timePointToUnsigned(TimePoint::max()) == std::numeric_limits<uint64_t>::max());

		// Ensure timePointFromUnsigned maps zero to TimePoint::min()
		static_assert(timePointFromUnsigned(0) == TimePoint::min());

		// Ensure timePointFromUnsigned maps UINT64_MAX to TimePoint::max()
		static_assert(timePointFromUnsigned(std::numeric_limits<uint64_t>::max()) == TimePoint::max());

		// Events with a period of 5ms and a phase offset of 0ms should happen at (5ms, 10ms, 15ms...)
		static_assert(computeNextEventTime(5'000'000, 0'000'000, 0'000'000U) == 5'000'000U);
		static_assert(computeNextEventTime(5'000'000, 0'000'000, 4'999'999U) == 5'000'000U);
		static_assert(computeNextEventTime(5'000'000, 0'000'000, 5'000'000U) == 10'000'000U);
		static_assert(computeNextEventTime(5'000'000, 0'000'000, 9'999'999U) == 10'000'000U);
		static_assert(computeNextEventTime(5'000'000, 0'000'000, 10'000'000U) == 15'000'000U);

		// Events with a period of 5ms and a phase offset of 1ms should happen at (1ms, 6ms, 11ms, ...)
		static_assert(computeNextEventTime(5'000'000, 1'000'000, 0'000'000U) == 1'000'000U);
		static_assert(computeNextEventTime(5'000'000, 1'000'000, 999'999U) == 1'000'000U);
		static_assert(computeNextEventTime(5'000'000, 1'000'000, 1'000'000U) == 6'000'000U);
		static_assert(computeNextEventTime(5'000'000, 1'000'000, 5'999'999U) == 6'000'000U);
		static_assert(computeNextEventTime(5'000'000, 1'000'000, 6'000'000U) == 11'000'000U);

		// Events with a period of 4ms and a phase offset of 1ms should happen at (1ms, 5ms, 9ms, ...)
		static_assert(computeNextEventTime(4'000'000, 1'000'000, 0'000'000U) == 1'000'000U);
		static_assert(computeNextEventTime(4'000'000, 1'000'000, 999'999U) == 1'000'000U);
		static_assert(computeNextEventTime(4'000'000, 1'000'000, 1'000'000U) == 5'000'000U);
		static_assert(computeNextEventTime(4'000'000, 1'000'000, 4'999'999U) == 5'000'000U);
		static_assert(computeNextEventTime(4'000'000, 1'000'000, 5'000'000U) == 9'000'000U);

		// Events with a period of 5ms and a phase offset of 2ms should happen at (2ms, 7ms, 12ms, ...)
		static_assert(computeNextEventTime(5'000'000, 2'000'000, 0'000'000U) == 2'000'000U);
		static_assert(computeNextEventTime(5'000'000, 2'000'000, 1'999'999U) == 2'000'000U);
		static_assert(computeNextEventTime(5'000'000, 2'000'000, 2'000'000U) == 7'000'000U);
		static_assert(computeNextEventTime(5'000'000, 2'000'000, 6'999'999U) == 7'000'000U);
		static_assert(computeNextEventTime(5'000'000, 2'000'000, 7'000'000U) == 12'000'000U);

		// Test the upper bound of the computation with forTimes near the uint64_t maximum of
		// UINT64_MAX (18'446'544'073'709'551'614).

		// Input values between the next to last representable event time and up the last representable
		// event time should compute the last representable event time.
		static_assert(computeNextEventTime(5'000'000, 2'000'000, 18'446'744'073'702'000'000U) ==
		18'446'744'073'707'000'000U);
		static_assert(computeNextEventTime(5'000'000, 2'000'000, 18'446'744'073'706'999'999U) ==
		18'446'744'073'707'000'000U);

		// Values greater from the last representable event time up to UINT64_MAX will unavoidably overflow
		// as the next event time is beyond UINT64_MAX. Instead a small truncated value will be returned.
		static_assert(computeNextEventTime(5'000'000, 2'000'000, 18'446'744'073'707'000'000U) == 2'448'384);
		static_assert(computeNextEventTime(5'000'000, 2'000'000, 18'446'744'073'709'551'615U) == 2'448'384);

		// If the next event time happens to be representable as UINT64_MAX, that value is returned.
		static_assert(computeNextEventTime(1'00'000, 551'615, 18'446'744'073'709'551'614U) ==
		18'446'744'073'709'551'615U);

		// NOLINTEND(*-magic-numbers)

		} // namespace

		[[nodiscard]] auto SingleDisplayRefreshSchedule::nextEvent(TimePoint forTime) const -> TimePoint {
		constexpr auto zero = TimePoint::duration(0);
		constexpr auto one = TimePoint::duration(1);
		auto delta = (forTime - base);

		// Ensure we will round "up" to the next event time. When forTime is greater than or equal
		// to the base time (equivalently delta >=0), the modulus operation rounds down towards
		// zero, and we just need to add the period to round to the next multiple. However when it
		// is less, and delta is negative, the rounding is up towards zero, except at the negative
		// multiples. When it is less, we instead need to add one to achieve the achieve the same
		// result, since the modulus done next rounds towards zero.
		delta += ((delta >= zero) ? period : one);

		// Compute first event time after forTime.
		return base + delta - delta % period;
		// The period must be positive (and not zero).
		CHECK_GT(period, TimePoint::duration(0));
		const auto periodNs = static_cast<uint64_t>(period.count());

		// Convert and reduce the base timestamp to a phase offset value in the interval [0, period) by
		// computing the remainder. This minimizes the overflow range.
		const auto phaseOffsetNs = timePointToUnsigned(base) % periodNs;

		// Convert the input timestamp.
		const auto forTimeNs = timePointToUnsigned(forTime);

		// Compute the next event time after the input time.
		const auto nextTimeNs = computeNextEventTime(periodNs, phaseOffsetNs, forTimeNs);

		// Ensure that the next time point is actually after the input time point. The only time this
		// check should trigger is for times near TimePoint::max() (317 years since the epoch), where
		// the computation of nextTimeNs exceeds TimePoint::max().
		CHECK_GT(nextTimeNs, forTimeNs);

		// Convert back to a signed TimePoint value.
		return timePointFromUnsigned(nextTimeNs);
		}

		auto toString(const SingleDisplayRefreshSchedule& schedule) -> std::string {

services/surfaceflinger/tests/end2end/tests/internal/DisplayRefresh_test.cpp

+75 −42

Original line number	Diff line number	Diff line
		@@ -14,7 +14,9 @@
		* limitations under the License.
		*/

		#include <bit>
		#include <chrono>
		#include <cstdint>
		#include <initializer_list>
		#include <map>
		#include <string>
		@@ -42,11 +44,11 @@ using MultiDisplayRefreshEventGenerator = test_framework::hwc3::MultiDisplayRefr
		// Helper to compute a monotonic clock time point from a integer value for the count of
		// nanoseconds since the epoch.
		constexpr auto operator""_ticks(unsigned long long value) -> TimePoint {
		return TimePoint{std::chrono::nanoseconds(value)};
		return TimePoint{std::chrono::nanoseconds(std::bit_cast<int64_t>(value))};
		}

		constexpr auto operator""_ms(unsigned long long value) -> std::chrono::milliseconds {
		return std::chrono::milliseconds(value);
		return std::chrono::milliseconds(std::bit_cast<int64_t>(value));
		}

		auto toString(const SingleDisplayRefreshEventGenerator::GenerateResult& result) -> std::string {
		@@ -62,64 +64,95 @@ auto toString(const MultiDisplayRefreshEventGenerator::GenerateResult& result) -
		}

		TEST(SingleDisplayRefreshSchedule, NextRefreshEventComputesNext) {
		// Set up a fixed refresh schedule where events happen every 5ms, anchored to a clock time of
		// 22ms. So refresh events happen at [..., 12ms, 17ms, 22ms, 27ms, 32ms, ...]
		static constexpr SingleDisplayRefreshSchedule schedule{
		.base = 22'000'000_ticks,
		.period = 5_ms,
		};

		struct TestCase {
		TimePoint input;
		TimePoint expected;
		int64_t input;
		int64_t expected;
		};

		// The implementation assumes that it is called with increasing input clock times.
		const auto testCases = std::initializer_list<TestCase>{
		// Clock times 1ms apart should always round up to an event time after the input time.
		// These values are to show that the rounding occurs to the 5ms refresh period.
		{.input = 12'000'000_ticks, .expected = 17'000'000_ticks},
		{.input = 13'000'000_ticks, .expected = 17'000'000_ticks},
		{.input = 14'000'000_ticks, .expected = 17'000'000_ticks},
		{.input = 15'000'000_ticks, .expected = 17'000'000_ticks},
		{.input = 16'000'000_ticks, .expected = 17'000'000_ticks},
		{.input = 17'000'000_ticks, .expected = 22'000'000_ticks},
		{.input = 18'000'000_ticks, .expected = 22'000'000_ticks},
		{.input = 19'000'000_ticks, .expected = 22'000'000_ticks},
		{.input = 20'000'000_ticks, .expected = 22'000'000_ticks},
		{.input = 21'000'000_ticks, .expected = 22'000'000_ticks}, // ^ Past events
		{.input = 22'000'000_ticks, .expected = 27'000'000_ticks}, // = At Base tme
		{.input = 23'000'000_ticks, .expected = 27'000'000_ticks}, // v Future events
		{.input = 24'000'000_ticks, .expected = 27'000'000_ticks},
		{.input = 25'000'000_ticks, .expected = 27'000'000_ticks},
		{.input = 26'000'000_ticks, .expected = 27'000'000_ticks},
		{.input = 27'000'000_ticks, .expected = 32'000'000_ticks},
		{.input = 28'000'000_ticks, .expected = 32'000'000_ticks},
		{.input = 29'000'000_ticks, .expected = 32'000'000_ticks},
		{.input = 30'000'000_ticks, .expected = 32'000'000_ticks},
		{.input = 31'000'000_ticks, .expected = 32'000'000_ticks},
		{.input = 12'000'000, .expected = 17'000'000},
		{.input = 13'000'000, .expected = 17'000'000},
		{.input = 14'000'000, .expected = 17'000'000},
		{.input = 15'000'000, .expected = 17'000'000},
		{.input = 16'000'000, .expected = 17'000'000},
		{.input = 17'000'000, .expected = 22'000'000},
		{.input = 18'000'000, .expected = 22'000'000},
		{.input = 19'000'000, .expected = 22'000'000},
		{.input = 20'000'000, .expected = 22'000'000},
		{.input = 21'000'000, .expected = 22'000'000}, // ^ Past events
		{.input = 22'000'000, .expected = 27'000'000}, // = At Base tme
		{.input = 23'000'000, .expected = 27'000'000}, // v Future events
		{.input = 24'000'000, .expected = 27'000'000},
		{.input = 25'000'000, .expected = 27'000'000},
		{.input = 26'000'000, .expected = 27'000'000},
		{.input = 27'000'000, .expected = 32'000'000},
		{.input = 28'000'000, .expected = 32'000'000},
		{.input = 29'000'000, .expected = 32'000'000},
		{.input = 30'000'000, .expected = 32'000'000},
		{.input = 31'000'000, .expected = 32'000'000},

		// Past clock times near the rounding point should correctly round up.
		{.input = 11'999'000_ticks, .expected = 12'000'000_ticks},
		{.input = 12'000'000_ticks, .expected = 17'000'000_ticks},
		{.input = 12'001'000_ticks, .expected = 17'000'000_ticks},
		{.input = 11'999'000, .expected = 12'000'000},
		{.input = 12'000'000, .expected = 17'000'000},
		{.input = 12'001'000, .expected = 17'000'000},

		// clock times near the base time should correctly round up.
		{.input = 21'999'000_ticks, .expected = 22'000'000_ticks},
		{.input = 22'000'000_ticks, .expected = 27'000'000_ticks},
		{.input = 22'001'000_ticks, .expected = 27'000'000_ticks},
		{.input = 21'999'000, .expected = 22'000'000},
		{.input = 22'000'000, .expected = 27'000'000},
		{.input = 22'001'000, .expected = 27'000'000},

		// Future clock times near the rounding point should correctly round up.
		{.input = 31'999'000_ticks, .expected = 32'000'000_ticks},
		{.input = 32'000'000_ticks, .expected = 37'000'000_ticks},
		{.input = 32'001'000_ticks, .expected = 37'000'000_ticks},
		{.input = 31'999'000, .expected = 32'000'000},
		{.input = 32'000'000, .expected = 37'000'000},
		{.input = 32'001'000, .expected = 37'000'000},

		};
		// TimePoint::min() (-9'223'372'036'854'775'808) should be accepted, and should compute
		// a correct value.
		{
		.input = TimePoint::min().time_since_epoch().count(),
		.expected = -9'223'372'036'853'000'000,
		},
		{
		.input = -9'223'372'036'853'000'001,
		.expected = -9'223'372'036'853'000'000,
		},
		{
		.input = -9'223'372'036'853'000'000,
		.expected = -9'223'372'036'848'000'000,
		},

		// Set up a fixed refresh schedule where events happen every 5ms, anchored to a clock time of
		// 22ms. So refresh events happen at [..., 12ms, 17ms, 22ms, 27ms, 32ms, ...]
		static constexpr SingleDisplayRefreshSchedule schedule{.base = 22'000'000_ticks,
		.period = 5_ms};
		// The largest valid input value is one less than the last representable event time.
		// Passing in the last representable event time would require returning an event time
		// greater than TimePoint::max() (9'223'372'036'854'775'807), which is not possible.
		{
		.input = 9'223'372'036'847'000'000, // The second to last representable event.
		.expected = 9'223'372'036'852'000'000, // The last representable event time.
		},
		{
		.input = 9'223'372'036'851'999'999, // The last representable event time - 1.
		.expected = 9'223'372'036'852'000'000, // The last representable event time.
		},
		// Note: The implementation asserts when the output value would not be representable,
		// rather than returning a truncated value.
		};

		for (const auto& testCase : testCases) {
		const auto actual = schedule.nextEvent(testCase.input);
		const auto actual = schedule.nextEvent(TimePoint{std::chrono::nanoseconds(testCase.input)})
		.time_since_epoch()
		.count();
		if (actual != testCase.expected) {
		FAIL() << fmt::format("For input: {} expected: {}, actual: {})",
		testCase.input.time_since_epoch(),
		testCase.expected.time_since_epoch(), actual.time_since_epoch());
		FAIL() << fmt::format("For input: {:} expected: {}, actual: {})", testCase.input,
		testCase.expected, actual);
		}
		}
		}