Closed Bug 1493715 Opened 6 years ago Closed 6 years ago

Signed integer overflow in jsdate.cpp

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(1 file)

https://searchfox.org/mozilla-central/rev/0640ea80fbc8d48f8b197cd363e2535c95a15eb3/js/src/jsdate.cpp#1184-1188 int n = c - '0'; while (i < length && '0' <= (c = s[i]) && c <= '9') { n = n * 10 + c - '0'; i++; } I haven't checked, but I think this string overflows `n` wherever `int` is 32 bits: js> new Date(Date.parse('4294967304/08/08')).toISOString() "2008-08-08T05:00:00.000Z" Signed int overflow is undefined behavior of a sort that nobody used to worry about. But these days, who knows? A C++ compiler could certainly look at the above code and conclude "n is definitely nonnegative after this loop (because It Can't Be that it would overflow)", and I can at least *imagine* getting different answers in opt vs. non-opt builds as a result.
Heh -- or, the compiler could deduce "ah, this loop will never run more than 10 times" and generate a fast-path copy of the loop body with the `i < length` check optimized away if `i + 10 < length` on entry... :) Exceedingly far-fetched and silly, but it could even be a security hole!
I can't currently dedicate time to a breaking change to date parsing, so this patch implements exactly what the previous code probably does already, but without the undefined behavior.
I think this is the same issue as in bug 866608, so if this fix is applied, bug 866608 can be closed, too.
Yep, same issue. Here is more *delightful* existing behavior that I am not going to fix in this bug: js> new Date(Date.parse("4294967295/4294967295/4294967295/4294967295/4294967295/4294967295/02/03/2004")).toISOString() "2004-02-03T06:00:00.000Z" The parser thinks this is fine (both with and without my patch) because 4294967295 parses as -1, which the function uses as a sentinel.
Comment on attachment 9011500 [details] Bug 1493715 - Use unsigned arithmetic for possibly overflowing DateParse computation. r?jwalden Jeff Walden [:Waldo] has approved the revision.
Attachment #9011500 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3acbdb6de90ef07169d80a25be50a226efcaae6b Bug 1493715 - Use unsigned arithmetic for possibly overflowing DateParse computation. r=jwalden
I pushed this directly to inbound (rather than use lando) because anba pushed bug 1420028 directly to inbound, and the two patches conflict.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee: nobody → jorendorff
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: