Bugzilla@Mozilla – Bug 524121
Date.setTime sets slots without checking for Date class
Last modified: 2010-01-25 00:07:21 PST
Summon comment box
My changes for bug 412296 removed the check for Date class in the implementation of Date.prototype.setTime method. This allows to overwrite slots with jsNaN value in arbitrary object. The following test case demonstrates the problem: ~/m/tm/js/src> cat ~/s/x.js var x = new Function('return 10'); Date.prototype.setTime.call(x); print(x()); ~/m/tm/js/src> ~/build/js32.tm.dbg/js ~/s/x.js Assertion failure: STOBJ_GET_CLASS(obj) == &js_DateClass, at /home/igor/m/tm/js/src/jsdate.cpp:1227 Trace/breakpoint trap
Created attachment 408034 [details] [review] fix v1 The fix is rather non-minimal but I wanted to ensure that I have not missed any more cases like this bug. The extra change removes double-boxing of time as jsdouble jsval in SetUTCTime callers and removal of date_constructor as the new SetUTCTime can serve its function just as well.
Comment on attachment 408034 [details] [review] fix v1 >@@ -1707,32 +1692,36 @@ date_makeDate(JSContext *cx, uintN maxar > uintN i; > jsdouble lorutime; /* local or UTC version of *date */ > jsdouble args[3], *argp, *stop; > jsdouble year, month, day; > jsdouble result; > > obj = JS_THIS_OBJECT(cx, vp); > if (!GetUTCTime(cx, obj, vp, &result)) >- return JS_FALSE; >+ return false; > > /* see complaint about ECMA in date_MakeTime */ >- if (argc == 0) >- return SetDateToNaN(cx, vp); >+ if (argc == 0) { >+ SetDateToNaN(cx, obj, NULL); No need for trailing NULL. >@@ -1813,46 +1798,46 @@ date_setYear(JSContext *cx, uintN argc, [snip] >+ if (!JSDOUBLE_IS_FINITE(year)) { >+ SetDateToNaN(cx, obj, vp); >+ return true; >+ } > > year = js_DoubleToInteger(year); > > if (!JSDOUBLE_IS_FINITE(result)) { > t = +0.0; > } else { > t = LocalTime(result); > } While you are here, may as well remove overbracing or even switch to y = ... ? ... : ...; >@@ -2586,17 +2525,17 @@ js_DateSetYear(JSContext *cx, JSObject * > MonthFromTime(local), > DateFromTime(local), > HourFromTime(local), > MinFromTime(local), > SecFromTime(local), > msFromTime(local)); > > /* SetUTCTime also invalidates local time cache. */ >- SetUTCTime(cx, obj, NULL, UTC(local)); >+ SetUTCTime(cx, obj, UTC(local)); Lame that these ancient (Netscape 2 era, for the server-side JS in "LiveWire") Friend APIs are void. File a followup bug? Good catch on the double boxing. /be
Created attachment 408219 [details] [review] fix v2 The new patch addresses the comment 2.
https://hg.mozilla.org/tracemonkey/rev/d31988919334
Regarding the severity of this bug: it allows, for example, to overwrite the slots that stores array's length and count with a pointer to jsdouble containing NaN. Typically such pointer, when reinterpreted, would represent relatively big number that significantly exceeds array bounds. So the bug allows to bypass array bound checks and read-write pretty much arbitrary value after malloced memory containing array's elements.
Created attachment 408264 [details] test case that date methods throw on incompatible objects The test case checks that all Date.prototype methods throw a TypeError when called with an incompatible this object.
this caused (I think) Bug 524671
http://hg.mozilla.org/mozilla-central/rev/d31988919334
(In reply to comment #7) > this caused (I think) Bug 524671 (It didn't, per that bug.) Is this patch ready for 1.9.1? Can you please request approval if so? Code freeze is scheduled for November 10 at 11:59pm.
See comment 5 regarding the danger of this bug.
Created attachment 411194 [details] [review] fix for 1.9.1 This backport to 1.9.1 includes the fix for the regression from the bug 527027.
Created attachment 411196 [details] delta between 192 and 191 This is a proof that a backport was trivial one and patch for 1.9.2 failed to apply to 1.9.1 due to code movements.
Comment on attachment 411194 [details] [review] fix for 1.9.1 Approved for 1.9.1.6, a=dveditz for release-drivers
I sometimes pipe a plain diff of two patches into grep -v '^[<>] ' to show the essential differences. /be
https://hg.mozilla.org/releases/mozilla-1.9.1/rev/18fb758d910b
Can we get this landed before code freeze tonight at 11:59pm PST?
(In reply to comment #16) > Can we get this landed before code freeze tonight at 11:59pm PST? The bug has already landed on 1.9.1, see comment 15.
Yeah, sorry. My query was messed up. :-/
Comment on attachment 411194 [details] [review] fix for 1.9.1 Ah ha! My query wasn't messed up. Dan approved this for 1.9.0 instead of 1.9.1. Switching approval flags to clean up queries.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/98b566cb970d
These bugs landed after b4 was cut. Moving flag out.
can we get some eyes on Bug 535550 to figure out if that new crash in 3.5.6 is a possible regression from this fix?