time.h: fix integer overflow in MsToAbsoluteTimespec() on 32-bit architectures #1042
Conversation
✅ Integration test succeeded!Requested by @dconeybe on commit ed0d970 |
app/src/time.h
Outdated
| (milliseconds * internal::kNanosecondsPerMillisecond); | ||
|
|
||
| t.tv_sec = nanoseconds / internal::kNanosecondsPerSecond; | ||
| t.tv_nsec = nanoseconds - (t.tv_sec * internal::kNanosecondsPerSecond); |
There was a problem hiding this comment.
Any reason to not use % ?
|
|
||
| TEST(TimeTests, MsToAbsoluteTimespecTest) { | ||
| const timespec t1 = firebase::internal::MsToAbsoluteTimespec(0); | ||
| const timespec t2 = firebase::internal::MsToAbsoluteTimespec(10000); |
There was a problem hiding this comment.
Do these value test for previous overflow scenarios?
If so a comment saying that this tests for overlfow on 32 bit systems would be helpful.
There was a problem hiding this comment.
As long as the test gets run on a 32-bit architecture it will detect the overflow. I'm not sure how our GitHub Actions are set up and if they trigger these tests on 32-bit platforms. I've opened a temporary PR #1043 to see if any of the GitHub Actions fail.
There was a problem hiding this comment.
I've also added this inline comment to time_test.cc:
This test verifies the fix for the old integer overflow bug on 32-bit architectures: #1042.
There was a problem hiding this comment.
I've verified the fix using GitHub Actions in #1043
…that MsToAbsoluteTimespecTest verifies
This PR fixes a long-standing bug in
MsToAbsoluteTimespec(int milliseconds)which would result in an integer overflow if invoked with a sufficiently-large "milliseconds". The bug manifested on 32-bit architectures because thetv_secandtv_nsecmembers oftimespecare 4 bytes each (32 bits), where on 64-bit architectures they are 8 bytes each (64 bits), and did not suffer from the overflow.The problem was that in
MsToAbsoluteTimespec(int milliseconds)it would add the given milliseconds, after converting to nanoseconds, to thetv_nsecmember, and then call a helper function to normalize the values; however, when being invoked with amillisecondsvalue of10000(10 seconds) this would causetv_nsecto overflow and wrap around to a negative value. When this malformedtimespecwas later specified tosem_timedwait()inSemaphore::TimedWait()it would cause the function call to fail immediately withEINVAL.The fix in this PR is to instead do all of the math with an
int64_tand then calculate thetv_secandtv_nsecvalues from it. By doing all of the math with a 64-bit integer, no overflow occurs.This fix will also fix a bunch of Firestore's transaction tests which are currently failing on 32-bit architectures due to this bug when they call
Future::Await(10000).Here is one example of a testapp crash due to this bug on an x86 Android emulator. Here is the implementation of
MsToAbsoluteTimespec()with the bug:firebase-cpp-sdk/app/src/time.h
Lines 89 to 95 in 05890ac
In this test, the
millisecondsargument was10000(10 seconds) and thetimespec thad the following values after the following lines:You can see that
tv_nsecis negative, which is invalid.