Tcl Source Code

clock-61.3 triggers an -fsanitize=signed-integer-overflow error:

generic/tclClock.c:472:52: runtime error: signed integer overflow: 9223372036854775807 + 210866803200 cannot be represented in type 'long long'

See attached patch. To avoid the overflow, I suggest dividing by SECONDS_PER_DAY first and then adding JULIAN_DAY_POSIX_EPOCH instead. The quotient also needs to be consistently rounded down, rather truncated towards 0 (GNU C and C99) or however a particular platform chooses (C89); otherwise incorrect results are produced for time values prior to the POSIX epoch. Not rounding down already produces incorrect results for time values prior to Julian day 0; the proposed clock-7.10 test currently fails with

-210866803200 -210866889600 -1 -210866803201 0 -210866803200 0 0 0

Thanks! Closing now.

I neglected how clock add … days takes -timezone into consideration. Specifying -timezone :UTC to clock add … days avoids the failure on OPTS=static,msvcrt, although whether GetSystemTimeZone is correct or unexpectedly :localtime is beyond the scope of this test. -timezone should likely also be specified in the test’s other clock add usage for consistency. See attached patch.

The exact cutoffs for localtime()/localtime_r() errors may depend on the platform epoch, but if struct tm is consistently defined, then I would think time values with years way outside of [1900+INT_MIN,1900+INT_MAX] will reliably cause an error. I will likely open other tickets about this.

However, the conditions under which localtime()/localtime_r() encounters errors (e.g. EOVERFLOW) are platform-dependent. So a test which uses these functions may not be portable.

I notice that ::tcl::clock::GetSystemTimeZone is returning :localtime when running tests for the OPTS=static,mscvrt configuration (rather than e.g. :America/Chicago), which causes ConvertUTCToLocal() to use ConvertUTCToLocalUsingC() instead of ConvertUTCToLocalUsingTable(). I am guessing this is not intentional.

However I wonder if clock-7.10 should still work with :localtime, or be constrained to not run under :localtime. Either way, having a similar test which forces :localtime could be useful.

Strange also, that all tests in 8.7 pass. Why does this fail in 8.6?

I have reproduced this locally and will take a look. Note that without the change to tclClock.c the test still fails for the same reason.

The new testcase fails in a OPTS=static,msvcrt build in Visual Studio, passes in all other configurations.

clock.test

==== clock-7.10 Julian Day, negative amount FAILED
==== Contents of test case:

# Note: %J does not accept negative input;
    # add negative amounts to Julian day 0 instead
    set s0 [clock scan 0 -format %J -gmt true]
    set J0 [scan [clock format $s0 -format %J -gmt true] %lld]
    set s0m1d [clock add $s0 -1 days]
    set s0m24h [clock add $s0 -24 hours]
    set J0m24h [scan [clock format $s0m24h -format %J -gmt true] %lld]
    set s0m1s [clock add $s0 -1 seconds]
    set J0m1s [scan [clock format $s0m1s -format %J -gmt true] %lld]
    list $s0m1d $s0m24h $J0m24h $s0m1s $J0m1s $s0 $J0  [::tcl::mathop::== $s0m1d $s0m24h] [::tcl::mathop::== $J0m24h $J0m1s]


---- Test generated error; Return code was: 1
---- Return code should have been one of: 0 2
---- errorInfo: localtime failed (clock value may be too large/small to represent)
    while executing
"return -options $opts $result"
    (procedure "::tcl::clock::add" line 98)
    invoked from within
"clock add $s0 -1 days"
    ("uplevel" body line 6)
    invoked from within
"uplevel 1 $script"
---- errorCode: CLOCK localtimeFailed
==== clock-7.10 FAILED

cmdAH.test

Fixed [ee677ecc84710dfd|here].

Many thanks!