GNOME Bugzilla – Bug 783841
test_GDateTime_new_from_timeval_overflow fails on 32 bit systems
Last modified: 2017-06-21 10:22:22 UTC
I just uploaded 2.53.2 to Ubuntu, where we run the installed-tests for all arches. We're seeing: # Start of new_from_timeval tests # Bug Reference: http://bugzilla.gnome.org/782089 ** GLib:ERROR:../../../../../glib/tests/gdatetime.c:417:test_GDateTime_new_from_timeval_overflow: 'dt' should be NULL # GLib:ERROR:../../../../../glib/tests/gdatetime.c:417:test_GDateTime_new_from_timeval_overflow: 'dt' should be NULL FAIL: glib/gdatetime.test (Child process killed by signal 6) on i386 and armhf (32 bit architectures). I don't know what all the overflow cases are here, but tv.tv_sec = G_MAXLONG; on 32 bit is going to be < G_MAXINT64, so the check in g_date_time_new_from_timeval doesn't trigger us to return NULL.
Created attachment 353888 [details] [review] gdatetime: Fix a potential overflow in overflow calculations I can’t remember whether glong (tv.tv_sec) needs to be explicitly promoted to gint64 here, or whether C does it automatically. Safer to make the cast explicit to avoid overflow issues on 32-bit platforms, where glong is 32-bit. Signed-off-by: Philip Withnall <withnall@endlessm.com>
Created attachment 353889 [details] [review] tests: Fix GDateTime overflow tests on 32-bit architectures On architectures where sizeof(glong) == 32 bits, there are no problems with overflow when constructing a GDateTime from a GTimeVal. Adjust the test for this by basing it on the maximum supported tv_sec value it can calculate, rather than a fixed ‘known unsupported’ value. Signed-off-by: Philip Withnall <withnall@endlessm.com>
I don’t have a 32-bit system to test these on, so if you could give them some review and a test run, that would be great. Thanks.
(In reply to Philip Withnall from comment #3) > I don’t have a 32-bit system to test these on, so if you could give them > some review and a test run, that would be great. Thanks. I'm running it now in an Ubuntu VM, and I think the test (test_GDateTime_new_from_timeval_overflow) is spinning. find_maximum_supported_tv_sec is alternating between (MSG: trying tv.tv_sec = 715827882, highest_success = 3579139413, lowest_failure = 2147483647) (MSG: trying tv.tv_sec = 3579139413, highest_success = 715827882, lowest_failure = 2147483647) Sorry, don't have time right this second to dig into it further. I can either over the weekend or on Monday morning if you'd like
Created attachment 354041 [details] [review] tests: Fix overflows in find_maximum_supported_tv_sec() The addition (highest_success + lowest_failure) could have overflowed, and typically would do on 32-bit platforms where the real highest_success should be G_MAXLONG. Fix that, and introduce special handling of the corner case of (highest_success = G_MAXLONG). Signed-off-by: Philip Withnall <withnall@endlessm.com>
(In reply to Iain Lane from comment #4) > (In reply to Philip Withnall from comment #3) > > I don’t have a 32-bit system to test these on, so if you could give them > > some review and a test run, that would be great. Thanks. > > I'm running it now in an Ubuntu VM, and I think the test > (test_GDateTime_new_from_timeval_overflow) is spinning. Oops, looks like there was an overflow problem in it. The latest attached patch should fix that but, as before, I’ve only been able to test it on a 64-bit machine.
(In reply to Philip Withnall from comment #5) > Created attachment 354041 [details] [review] [review] > tests: Fix overflows in find_maximum_supported_tv_sec() > > The addition (highest_success + lowest_failure) could have overflowed, > and typically would do on 32-bit platforms where the real > highest_success should be G_MAXLONG. Fix that, and introduce special > handling of the corner case of (highest_success = G_MAXLONG). > > Signed-off-by: Philip Withnall <withnall@endlessm.com> This seems to make sense to me, and it makes the tests pass, thanks! I just applied it manually and ran the one test; doing a full build now and will report back if it fails. If I don't comment again, then you can assume it worked.
I’m assuming that’s all fine then. Thanks for testing! Attachment 353888 [details] pushed as 2db7aa4 - gdatetime: Fix a potential overflow in overflow calculations Attachment 353889 [details] pushed as 30fed3b - tests: Fix GDateTime overflow tests on 32-bit architectures Attachment 354041 [details] pushed as 428acd9 - tests: Fix overflows in find_maximum_supported_tv_sec()