Skip to content

Commit

Permalink
Fix undefined behaviour in aos/time/time.cc
Browse files Browse the repository at this point in the history
We're currently seeing the following undefined behaviour in the code
that converts `realtime_clock::time_point` values into human-readable
strings. The issue is that the computation overflows what we can
represent in 64 bits when stringifying `realtime_clock::min_time`.

    $ bazel test --config=ubsan @aos//aos/events/logging:logfile_utils_test --test_filter=PartsMessageReaderDeathTest.TooFarOutOfOrder --test_output=streamed   --test_env=UBSAN_SYMBOLIZER_PATH="$(readlink -f external/llvm_k8/bin/llvm-symbolizer)"
    ...
    [ RUN      ] PartsMessageReaderDeathTest.TooFarOutOfOrder
    external/aos/aos/events/logging/logfile_utils_test.cc:169: Failure
    Death test: { reader.ReadMessage(); }
        Result: died but not with expected error.
      Expected: contains regular expression "-0.000000001sec vs. 0.000000000sec"
    Actual msg:
    [  DEATH   ] external/llvm_k8/include/c++/v1/__chrono/duration.h:102:59: runtime error: signed integer overflow: -9223372037 * 1000000000 cannot be represented in type '_Ct' (aka 'long long')
    ...
    [  DEATH   ]     #4 0x7f6788f54d9a in aos::operator<<(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, std::__1::chrono::time_point<aos::realtime_clock, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000000000l>>> const&) external/aos/aos/time/time.cc:189:55
    [  DEATH   ]     #5 0x7f67938da571 in aos::logger::operator<<(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, aos::logger::LogParts const&) external/aos/aos/events/logging/logfile_sorting.cc:2163:50
    [  DEATH   ]     #6 0x7f6793aeec53 in aos::logger::PartsMessageReader::ReadMessage() external/aos/aos/events/logging/logfile_utils.cc:1679:32
    [  DEATH   ]     #7 0x55ed45105cb0 in aos::logger::testing::PartsMessageReaderDeathTest_TooFarOutOfOrder_Test::TestBody() external/aos/aos/events/logging/logfile_utils_test.cc:169:3
    ...
    [  DEATH   ]
    [  DEATH   ] SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior external/llvm_k8/include/c++/v1/__chrono/duration.h:102:59 in
    [  DEATH   ]
    [  FAILED  ] PartsMessageReaderDeathTest.TooFarOutOfOrder (200 ms)
    [----------] 1 test from PartsMessageReaderDeathTest (200 ms total)

This patch fixes the issue by detecting potential overflow and return
a mostly non-human-readable string version of the time stamp.

Change-Id: I4bff6272ac6f4d48c646face2c13fbc7dd3bb0ed
Signed-off-by: James Kuszmaul <james.kuszmaul@bluerivertech.com>
  • Loading branch information
philsc authored and jameskuszmaul-brt committed Feb 16, 2024
1 parent 7d4b98d commit d0d264d
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 2 deletions.
10 changes: 10 additions & 0 deletions aos/time/time.cc
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,16 @@ std::ostream &operator<<(std::ostream &stream,
: std::chrono::duration_cast<std::chrono::seconds>(
now.time_since_epoch());

// We can run into some corner cases where the seconds value is large enough
// to cause the conversion to nanoseconds to overflow. That is undefined
// behaviour so we prevent it with this check here.
if (int64_t result;
__builtin_mul_overflow(seconds.count(), 1'000'000'000, &result)) {
stream << "(unrepresentable realtime " << now.time_since_epoch().count()
<< ")";
return stream;
}

std::time_t seconds_t = seconds.count();
stream << std::put_time(localtime_r(&seconds_t, &tm), "%Y-%m-%d_%H-%M-%S.")
<< std::setfill('0') << std::setw(9)
Expand Down
5 changes: 3 additions & 2 deletions aos/time/time_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,9 @@ TEST(TimeTest, OperatorStreamRealtimeNegative) {
std::stringstream s;
s << t;

EXPECT_EQ(s.str(), "1677-09-21_00-12-43.145224192");
EXPECT_EQ(realtime_clock::FromString(s.str()).value(), t);
// min_time happens to be unrepresentable because of rounding and signed
// integer overflow.
EXPECT_EQ(s.str(), "(unrepresentable realtime -9223372036854775808)");
}

{
Expand Down

0 comments on commit d0d264d

Please sign in to comment.