code review 10328043: time: handle integer overflow in Sub

time: handle integer overflow in Sub

If time.Sub results in a value that won't fit in a Duration (int64),
return either the min or max int64 value as appropriate.

Fixes issue 5011.

[rick, 2013-06-17 00:00:48]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2013-06-18 00:35:29]

R=rsc

[r, 2013-06-18 00:47:11]

https://codereview.appspot.com/10328043/diff/2002/src/pkg/time/time.go
File src/pkg/time/time.go (right):

https://codereview.appspot.com/10328043/diff/2002/src/pkg/time/time.go#newcod...
src/pkg/time/time.go:624: if u.sec > 0 && t.sec < minDuration+u.sec {
this doesn't look right. you're adding nanoseconds and seconds.
why don't you give minDuration and maxDuration types to catch things like this?

i'd start by splitting the code into two blocks, one where t >= u and one where
t < u.
do the math
if you end up on the wrong side, just return the limit

[rsc, 2013-06-21 19:00:08]

https://codereview.appspot.com/10328043/diff/2002/src/pkg/time/time.go
File src/pkg/time/time.go (right):

https://codereview.appspot.com/10328043/diff/2002/src/pkg/time/time.go#newcod...
src/pkg/time/time.go:427: const (
Delete from here.

https://codereview.appspot.com/10328043/diff/2002/src/pkg/time/time.go#newcod...
src/pkg/time/time.go:428: minDuration = -1 << 63
minDuration Duration = -1<<63
maxDuration Duration = 1<<63 - 1

https://codereview.appspot.com/10328043/diff/2002/src/pkg/time/time.go#newcod...
src/pkg/time/time.go:624: if u.sec > 0 && t.sec < minDuration+u.sec {
There are lots of ways this code can go wrong trying not to compute a wrong
answer.
Compute the answer and check whether it's right.

d := Duration(t.sec-u.sec)*Second + Duration(t.nsec-u.nsec)
switch {
case u.Add(d).Equal(t):
    return d // d is correct
case t.Before(u):
    return minDuration // t - u is negative out of range
default:
    return maxDuration // t - u is positive out of range
}

[rsc, 2013-06-21 19:00:34]

https://codereview.appspot.com/10328043/diff/2002/src/pkg/time/time.go
File src/pkg/time/time.go (right):

https://codereview.appspot.com/10328043/diff/2002/src/pkg/time/time.go#newcod...
src/pkg/time/time.go:427: const (
On 2013/06/21 19:00:09, rsc wrote:
> Delete from here.

Sorry, this comment was supposed to have been removed.
Keep the const block but adjust as suggested below.

[rick, 2013-06-21 21:24:13]

I forgot to mail when I made comments earlier, sorry. Do I need to worry about
unspecified overflow behavior?

I will move forward using rsc's comments.

Thanks.

https://codereview.appspot.com/10328043/diff/2002/src/pkg/time/time.go
File src/pkg/time/time.go (right):

https://codereview.appspot.com/10328043/diff/2002/src/pkg/time/time.go#newcod...
src/pkg/time/time.go:624: if u.sec > 0 && t.sec < minDuration+u.sec {
On 2013/06/18 00:47:11, r wrote:
> this doesn't look right. you're adding nanoseconds and seconds.
> why don't you give minDuration and maxDuration types to catch things like
this?

I wanted to use Math.MinInt64 and Math.MaxInt64 but didn't want to add the
dependency so I called it minDuration and maxDuration. In retrospect I guess
that's a bit confusing. Do you think I should create a separate maxSeconds and
minSeconds with the same values (and change min/maxDuration to Durations)? 

> i'd start by splitting the code into two blocks, one where t >= u and one
where
> t < u.
> do the math
> if you end up on the wrong side, just return the limit

I based the code in this CL on recommendations for C code since overflow
behavior there is undefined and I was worried about ARM CPUs, which I know
little about. Is Go code guaranteed to overflow "correctly" always?

Thanks.

[rick, 2013-06-21 21:35:26]

PTAL

[r, 2013-06-21 21:37:15]

https://codereview.appspot.com/10328043/diff/11001/src/pkg/time/time.go
File src/pkg/time/time.go (right):

https://codereview.appspot.com/10328043/diff/11001/src/pkg/time/time.go#newco...
src/pkg/time/time.go:624: d := Duration(t.sec-u.sec)*Second +
Duration(t.nsec-u.nsec)
// Check for overflow or underflow.

https://codereview.appspot.com/10328043/diff/11001/src/pkg/time/time.go#newco...
src/pkg/time/time.go:632: }
you need to return something, which you don't, which means this hasn't been run.

[rick, 2013-06-21 21:44:52]

PTAL

https://codereview.appspot.com/10328043/diff/11001/src/pkg/time/time.go
File src/pkg/time/time.go (right):

https://codereview.appspot.com/10328043/diff/11001/src/pkg/time/time.go#newco...
src/pkg/time/time.go:624: d := Duration(t.sec-u.sec)*Second +
Duration(t.nsec-u.nsec)
On 2013/06/21 21:37:15, r wrote:
> // Check for overflow or underflow.

Done.

https://codereview.appspot.com/10328043/diff/11001/src/pkg/time/time.go#newco...
src/pkg/time/time.go:632: }
On 2013/06/21 21:37:15, r wrote:
> you need to return something, which you don't, which means this hasn't been
run.

What do you mean? There is a default case for the switch and I ran "go test" on
the test package and all.bash before uploading.

[r, 2013-06-21 21:53:18]

https://codereview.appspot.com/10328043/diff/11001/src/pkg/time/time.go
File src/pkg/time/time.go (right):

https://codereview.appspot.com/10328043/diff/11001/src/pkg/time/time.go#newco...
src/pkg/time/time.go:632: }
my mistake, sorry. the colors confuse me sometimes.

https://codereview.appspot.com/10328043/diff/17001/src/pkg/time/time_test.go
File src/pkg/time/time_test.go (right):

https://codereview.appspot.com/10328043/diff/17001/src/pkg/time/time_test.go#...
src/pkg/time/time_test.go:1359: t.Errorf("%d: got %d; want %d", i, got, st.d)
it would be a service (and easy) to print this more usefully (but check my
typing by introducing a failure and/or running vet)

"#%d: Sub(%v, %v): got %v; want %v", i, st.t, st.u, got, st.d)

[rick, 2013-06-22 00:56:34]

PTAL

https://codereview.appspot.com/10328043/diff/17001/src/pkg/time/time_test.go
File src/pkg/time/time_test.go (right):

https://codereview.appspot.com/10328043/diff/17001/src/pkg/time/time_test.go#...
src/pkg/time/time_test.go:1359: t.Errorf("%d: got %d; want %d", i, got, st.d)
On 2013/06/21 21:53:19, r wrote:
> it would be a service (and easy) to print this more usefully (but check my
> typing by introducing a failure and/or running vet)
> 
> "#%d: Sub(%v, %v): got %v; want %v", i, st.t, st.u, got, st.d)

Done. Here's a sample failure:

        time_test.go:1359: #1: Sub(2009-11-23 00:00:00.000000001 +0000 UTC,
2009-11-23 00:00:00 +0000 UTC): got 1ns; want 10ns

[r, 2013-06-22 01:05:41]

LGTM

[r, 2013-06-22 01:08:01]

*** Submitted as https://code.google.com/p/go/source/detail?r=56909cb770fe ***

time: handle integer overflow in Sub

If time.Sub results in a value that won't fit in a Duration (int64),
return either the min or max int64 value as appropriate.

Fixes issue 5011.

R=golang-dev, bradfitz, r, rsc
CC=golang-dev
https://codereview.appspot.com/10328043

Committer: Rob Pike <r@golang.org>