Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit e495d99b authored by Lloyd Pique's avatar Lloyd Pique
Browse files

end2end: Fix UB in refresh event time computations [10/N] 

The function to compute the next event time could overflow near
INT64_MIN and INT64_MAX, and as the values were signed, the result was
undefined behavior, leading to errors that were caught later.

This was problematic as other part of the generator assumes INT64_MIN
(via std::chrono::steady_clock::time_point::min()) is a reasonable
choice for an initial "last emitted event time" when first starting up.

To fix this, the function was re-implemented to use uint64_t internally,
and adjusted to avoid underflow completely, and overflow as much as
possible. Overflow is still however possible when the next event time is
greater than time_point::max(), and this is now caught with a CHECK(),
which aborts the process. In normal use, this could only be a problem
317 years after the last boot, as that is when the clock count starts on
Linux. Because of that, it should not cause a problem in usage in tests,
and probably would not even be an issue in production.

The internal tests now covers values at the extreme values where
possible to ensure those values work.

Flag: TEST_ONLY
Bug: 372735083
Test: atest surfaceflinger_end2end_tests

Change-Id: I6db126cbeeb2dc58e9a379f53925d1d9f250102b
parent a7b22cf3

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);

        }

    }

}