Closed
Bug 1493715
Opened 6 years ago
Closed 6 years ago
Signed integer overflow in jsdate.cpp
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
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.
Assignee | ||
Comment 1•6 years ago
|
||
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!
Assignee | ||
Comment 2•6 years ago
|
||
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.
Comment 3•6 years ago
|
||
I think this is the same issue as in bug 866608, so if this fix is applied, bug 866608 can be closed, too.
Assignee | ||
Comment 4•6 years ago
|
||
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 5•6 years ago
|
||
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+
Assignee | ||
Comment 6•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3acbdb6de90ef07169d80a25be50a226efcaae6b
Bug 1493715 - Use unsigned arithmetic for possibly overflowing DateParse computation. r=jwalden
Assignee | ||
Comment 7•6 years ago
|
||
I pushed this directly to inbound (rather than use lando) because anba pushed bug 1420028 directly to inbound, and the two patches conflict.
Comment 8•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•6 years ago
|
Assignee: nobody → jorendorff
You need to log in
before you can comment on or make changes to this bug.
Description
•