time.h: fix integer overflow in MsToAbsoluteTimespec() on 32-bit architectures

[dconeybe]

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

	inline timespec MsToAbsoluteTimespec(int milliseconds) {
	timespec t;
	clock_gettime(CLOCK_REALTIME, &t);
	t.tv_nsec += milliseconds * internal::kNanosecondsPerMillisecond;
	NormalizeTimespec(&t);
	return t;
	}

In this test, the milliseconds argument was 10000 (10 seconds) and the timespec t had the following values after the following lines:

• After line 91: tv_sec=1659589211 tv_nsec=799274196
• After line 92: tv_sec=1659589211 tv_nsec=-2085627692
• After line 93: tv_sec=1659589209 tv_nsec=-85627692

You can see that tv_nsec is negative, which is invalid.

[github-actions]

✅  Integration test succeeded!

Requested by @dconeybe on commit ed0d970
Last updated: Fri Aug 5 15:04 PDT 2022
View integration test log & download artifacts

[a-maurice]

Any reason to not use % ?

[dconeybe]

Good idea. Done.

[DellaBitta]

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.

[dconeybe]

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.

[dconeybe]

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.

[dconeybe]

I've verified the fix using GitHub Actions in #1043