time.h: fix integer overflow in MsToAbsoluteTimespec() on 32-bit architectures - #1042
time.h: fix integer overflow in MsToAbsoluteTimespec() on 32-bit architectures#1042dconeybe merged 6 commits into
Conversation
Sorry, something went wrong.
✅ Integration test succeeded!Requested by @dconeybe on commit ed0d970 |
Sorry, something went wrong.
| (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 % ?
Sorry, something went wrong.
There was a problem hiding this comment.
Good idea. Done.
Sorry, something went wrong.
|
|
||
| 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
I've verified the fix using GitHub Actions in #1043
Sorry, something went wrong.
…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 the tv_sec and tv_nsec members of timespec are 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 the tv_nsec member, and then call a helper function to normalize the values; however, when being invoked with a milliseconds value of 10000 (10 seconds) this would cause tv_nsec to overflow and wrap around to a negative value. When this malformed timespec was later specified to sem_timedwait() in Semaphore::TimedWait() it would cause the function call to fail immediately with EINVAL.
The fix in this PR is to instead do all of the math with an int64_t and then calculate the tv_sec and tv_nsec values 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 milliseconds argument was 10000 (10 seconds) and the timespec t had the following values after the following lines:
You can see that tv_nsec is negative, which is invalid.