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



14 years ago
14 years ago


(Reporter: aleksey, Assigned: wtc)


Firefox Tracking Flags

(Not tracked)



(1 attachment)



14 years ago
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

14 years ago
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.
Attachment #207572 - Flags: review?(darin)

Comment 2

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

Comment 3

14 years ago
Comment on attachment 207572 [details] [diff] [review]
Proposed patch

Aleksey, could you review this patch?
Attachment #207572 - Flags: review?(aleksey)

Comment 4

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


14 years ago
Attachment #207572 - Flags: review?(aleksey) → review+

Comment 5

14 years ago
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

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

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
Last Resolved: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7

Comment 6

14 years ago
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

14 years ago
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?
Attachment #207572 - Flags: review?(darin) → review+
You need to log in before you can comment on or make changes to this bug.