From 3334cf675a3862a013135f1ce16bb46dea1cc493 Mon Sep 17 00:00:00 2001 From: Ben Wiederhake Date: Sun, 1 Jan 2023 20:07:27 +0100 Subject: [PATCH] AK+Kernel: Eliminate UB (signed overflow) from days_since_epoch --- AK/Time.h | 15 ++++++++------- Kernel/Arch/x86_64/RTC.cpp | 4 ++-- Tests/AK/TestTime.cpp | 17 ++++++++++------- 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/AK/Time.h b/AK/Time.h index d301f5b6e39..6d6ad16763a 100644 --- a/AK/Time.h +++ b/AK/Time.h @@ -70,9 +70,9 @@ namespace Detail { // Integer division rounding towards negative infinity. // TODO: This feels like there should be an easier way to do this. template -constexpr int floor_div_by(int dividend) +constexpr i64 floor_div_by(i64 dividend) { - static_assert(divisor >= 1); + static_assert(divisor > 1); int is_negative = dividend < 0; return (dividend + is_negative) / divisor - is_negative; } @@ -80,13 +80,13 @@ constexpr int floor_div_by(int dividend) // Counts how many integers n are in the interval [begin, end) with n % positive_mod == 0. // NOTE: "end" is not considered to be part of the range, hence "[begin, end)". template -constexpr int mod_zeros_in_range(int begin, int end) +constexpr i64 mod_zeros_in_range(i64 begin, i64 end) { return floor_div_by(end - 1) - floor_div_by(begin - 1); } } -constexpr int years_to_days_since_epoch(int year) +constexpr i64 years_to_days_since_epoch(int year) { int begin_year, end_year, leap_sign; if (year < 1970) { @@ -98,17 +98,18 @@ constexpr int years_to_days_since_epoch(int year) end_year = year; leap_sign = +1; } + i64 year_i64 = year; // This duplicates the logic of 'is_leap_year', with the advantage of not needing any loops. // Given that the definition of leap years is not expected to change, this should be a good trade-off. - int days = 365 * (year - 1970); - int extra_leap_days = 0; + i64 days = 365 * (year_i64 - 1970); + i64 extra_leap_days = 0; extra_leap_days += Detail::mod_zeros_in_range<4>(begin_year, end_year); extra_leap_days -= Detail::mod_zeros_in_range<100>(begin_year, end_year); extra_leap_days += Detail::mod_zeros_in_range<400>(begin_year, end_year); return days + extra_leap_days * leap_sign; } -constexpr int days_since_epoch(int year, int month, int day) +constexpr i64 days_since_epoch(int year, int month, int day) { return years_to_days_since_epoch(year) + day_of_year(year, month, day); } diff --git a/Kernel/Arch/x86_64/RTC.cpp b/Kernel/Arch/x86_64/RTC.cpp index 7af7c9637af..e7a8cd48f82 100644 --- a/Kernel/Arch/x86_64/RTC.cpp +++ b/Kernel/Arch/x86_64/RTC.cpp @@ -112,8 +112,8 @@ time_t now() dmesgln("RTC: {} Year: {}, month: {}, day: {}, hour: {}, minute: {}, second: {}", (did_read_rtc_successfully ? "" : "(failed to read)"), year, month, day, hour, minute, second); - time_t days_since_epoch = years_to_days_since_epoch(year) + day_of_year(year, month, day); - return ((days_since_epoch * 24 + hour) * 60 + minute) * 60 + second; + time_t days = days_since_epoch(year, month, day); + return ((days * 24 + hour) * 60 + minute) * 60 + second; } } diff --git a/Tests/AK/TestTime.cpp b/Tests/AK/TestTime.cpp index 94ad5bbcb32..caafbdab95f 100644 --- a/Tests/AK/TestTime.cpp +++ b/Tests/AK/TestTime.cpp @@ -277,12 +277,12 @@ TEST_CASE(is_negative) struct YearAndDays { int year; - int days; + i64 days; }; TEST_CASE(years_to_days_since_epoch_points) { - Array test_data = { { + Array test_data = { { { 1969, -365 }, { 1970, 0 }, { 1971, 365 }, @@ -306,11 +306,14 @@ TEST_CASE(years_to_days_since_epoch_points) { 5881474, 2147444740 }, // Very important year: https://github.com/SerenityOS/serenity/pull/16760#issuecomment-1369054745 { -999999, -365961662 }, + // The following two values haven't been verified by any other algorithm, but are very close to "year * 365.2425", and prove that there is no UB due to signed overflow: + { 2147483647, 784351576412 }, + { -2147483648, -784353015833 }, } }; for (auto entry : test_data) { int year = entry.year; - int expected_days = entry.days; - int actual_days = years_to_days_since_epoch(year); + i64 expected_days = entry.days; + i64 actual_days = years_to_days_since_epoch(year); EXPECT_EQ(actual_days, expected_days); } } @@ -319,7 +322,7 @@ BENCHMARK_CASE(years_to_days_since_epoch_benchmark) { // This benchmark takes consistently "0ms" on Linux, and "0ms" on Serenity. for (size_t i = 0; i < 100; ++i) { - int actual_days = years_to_days_since_epoch(-5877640); + i64 actual_days = years_to_days_since_epoch(-5877640); (void)actual_days; EXPECT_EQ(actual_days, -2147483456); } @@ -467,8 +470,8 @@ TEST_CASE(years_to_days_since_epoch_span) // clang-format on for (size_t offset = 0; offset < test_data.size(); ++offset) { int year = offset + test_data_start_year; - int expected_days = test_data[offset]; - int actual_days = years_to_days_since_epoch(year); + i64 expected_days = test_data[offset]; + i64 actual_days = years_to_days_since_epoch(year); EXPECT_EQ(actual_days, expected_days); } }