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)
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)
The attached patch works fine for me and it does make perfect sense.
Comment on attachment 207572 [details] [diff] [review] Proposed patch Aleksey, could you review this patch?
Attachment #207572 - Flags: review?(aleksey)
Yes, this patch looks good to me. I already reviewed it for myself :)
Attachment #207572 - Flags: review?(aleksey) → review+
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: 188.8.131.52; previous revision: 184.108.40.206 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.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7
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 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.