322422 - PR_ExplodeTime() returns wrong result for negative time between -1 sec and 0

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Aleksey Sanin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

I believe I found a bug in NSPR PR_ExplodeTime() function for
negative time between -1 sec and 0 but I would like to confirm it
with someone who knows NSPR code better than I do:

PR_ExplodeTime(-1, PR_GMTParameters, &et)

returns correct value "1969/12/31 23:59:59.999999" but same
call with local time parameter

PR_ExplodeTime(-1, PR_PR_LocalTimeParameters, &et)

returns "1969/12/31 16:00:00.999999" instead of
"1969/12/31 15:59:59.999999" (I am in CA, thus the 8 hours
difference from GMT).

At the moment, the code uses MT_safe_localtime() around line 706
in file prtime.c to figure out if the date is before midnight
January 1, 1970 GMT. But the resolution for Windows call is seconds
and value between -1 sec and 0 is converted to 0 secs thus this call
returns incorrect result. The proposed fix would be to go ahead and
check if secs64 is negative as soon as possible (around line 691)
and return the offset2Jan1970 right there.


Reproducible: Always

Steps to Reproduce:
call PR_ExplodeTime(-1, PR_PR_LocalTimeParameters, &et)
Actual Results:  
1969/12/31 16:00:00.999999

Expected Results:  
1969/12/31 15:59:59.999999 (8 hours diff from GMT for local time zone)

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

Yes, Aleksey, your analysis of the bug is correct.
Thank you very much.

The problem is that the value between -1 sec and 0
should be converted to -1 sec, and more generally,
a non-integer value should be truncated *down* to
the first (greatest) integer less than the value,
whether the value is positive or negative.  The
reason we need to truncating down is that -1 usec
should be represented (in the "exploded time" form)
as (the equivalent of) -1 sec and 999999 usec.

On most platforms, dividing a negative integer
discards the negative fraction, which is equivalent
to truncating *up*.  (I also reproduced this bug
on Linux, which is why I needed a more general fix
than your proposed fix.)

This patch makes sure we truncate down when we divide
secs64 by usecPerSec, even if secs64 is negative.

Comment 2

[Aleksey Sanin]

The attached patch works fine for me and it does make perfect sense.

Comment 3

[Wan-Teh Chang]

Comment on attachment 207572 [details] [diff] [review]
Proposed patch

Aleksey, could you review this patch?

Comment 4

[Aleksey Sanin]

Yes, this patch looks good to me. I already reviewed it for myself :)

Comment 5

[Wan-Teh Chang]

I checked in the patch on the NSPR trunk (NSPR 4.7) and
the NSPRPUB_PRE_4_2_CLIENT_BRANCH (Gecko 1.6 alpha).

Checking in prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.23; previous revision: 3.22
done

Checking in prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.12.2.11; previous revision: 3.12.2.10
done

Aleksey, can you work around or tolerate this bug, or
do you need it fixed sooner in NSPR 4.6.2?

To work around this bug, do not pass a negative PRTime
value with fractional seconds to PR_ExplodeTime.  You'd
need to do something similar to my patch: given a negative
PRTime, you round it *downwards* to seconds, pass that
to PR_ExplodeTime, and then set et.tm_usec to the *positive*
fractional seconds.  Please let me know if this is not
clear.

Comment 6

[Aleksey Sanin]

No, I don't need it urgently. I can easily apply patches to NSPR during my build process :)

Thanks a lot for very quick fix!

Comment 7

[Darin Fisher]

Comment on attachment 207572 [details] [diff] [review]
Proposed patch

r=darin, why not drop the LL_ macros and just use raw int64 arithmetic?  we do that elsewhere in NSPR, so why not here?