Closed
Bug 315509
Opened 19 years ago
Closed 19 years ago
Crash: array_unshift doesn't handle holes properly [@ js_DeleteProperty - array_unshift]
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.9alpha1
People
(Reporter: moz_bug_r_a4, Assigned: moz_bug_r_a4)
References
Details
(4 keywords)
Crash Data
Attachments
(4 files, 1 obsolete file)
355 bytes,
text/html
|
Details | |
1.21 KB,
text/html
|
Details | |
1.33 KB,
patch
|
mrbkap
:
review+
dveditz
:
approval1.8.0.1+
dveditz
:
approval1.8.1+
|
Details | Diff | Splinter Review |
627 bytes,
text/html
|
Details |
This was caused by the fix for Bug 310425. http://lxr.mozilla.org/mozilla/source/js/src/jsarray.c#1099 1099 array_unshift(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, ... 1117 if (id == JSID_HOLE) { 1118 OBJ_DELETE_PROPERTY(cx, obj, id2, &junk); 1119 continue; 1120 } Here, id2 is not initialized(*A), or holds last used value(*B). *A When the last element of an array is hole, uninitialized id2 is passed to js_DeleteProperty as id arg. http://lxr.mozilla.org/mozilla/source/js/src/jsobj.c#3198 3198 js_DeleteProperty(JSContext *cx, JSObject *obj, jsid id, jsval *rval) ... 3215 CHECK_FOR_STRING_INDEX(id); http://lxr.mozilla.org/mozilla/source/js/src/jsobj.c#2243 2243 #define CHECK_FOR_STRING_INDEX(id) 2244 JS_BEGIN_MACRO 2245 if (JSID_IS_ATOM(id)) { ... If (id & 3) == 0, id is treated as JSAtom. And then Firefox attempts to read from unexpected addresses and might cause Segmentation fault. *B var a = [0,1,2,3,4]; delete a[1]; var s = "before: " + a + "\n"; a.unshift("a","b"); s += " after: " + a + "\n"; before: 0,,2,3,4 after: a,b,0,3,,3,4
Assignee | ||
Comment 1•19 years ago
|
||
Assignee | ||
Comment 2•19 years ago
|
||
Assignee | ||
Comment 3•19 years ago
|
||
Comment 4•19 years ago
|
||
Comment on attachment 202209 [details] [diff] [review] patch Yes, control-flow-statements (such as continue, break, and return) are most certainly evil around holes in arrays. r=mrbkap
Attachment #202209 -
Flags: review+
Updated•19 years ago
|
Assignee: general → moz_bug_r_a4
Flags: testcase?
Comment 5•19 years ago
|
||
Fix checked into trunk. Nominating for 1.8.1 since we probably would want this sort of fix.
Status: NEW → RESOLVED
Closed: 19 years ago
Flags: blocking1.8.1?
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Comment 6•19 years ago
|
||
*** Bug 316208 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Keywords: crash
Summary: Crash: array_unshift doesn't handle holes properly → Crash: array_unshift doesn't handle holes properly [@ js_DeleteProperty]
Comment 7•19 years ago
|
||
Any chance this can land on 1.8.0 so that we don't have to wait for 1.8.1?
Comment 8•19 years ago
|
||
(In reply to comment #7) > Any chance this can land on 1.8.0 so that we don't have to wait for 1.8.1? I am arguing for that, after failing to pursue it earlier based on likely minus'ing by drivers. But that was almost a week ago, and this patch could have landed without harm since then, for inclusion in a respin. Do you have JS, or know of any, that this bug bites? /be
Comment 9•19 years ago
|
||
Comment 10•19 years ago
|
||
Comment on attachment 203005 [details] Testcase for workaround using splice ><HTML><HEAD/><BODY><PRE id="p">Actual Results (unshift): > before: 0,,2,3,4 > after: a,b,0,3,,3,4 ></PRE> ><PRE id="y">Actual Results(split): > before: 0,,2,3,4 > after: a,b,0,,2,3,4 ></PRE> ><PRE>Expected Results: > before: 0,,2,3,4 > after: a,b,0,,2,3,4 ></PRE> > ><SCRIPT> >function x() { > var a = [0,1,2,3,4]; > delete a[1]; > var s = " before: " + a + "\n"; > a.unshift("a","b"); > s += " after: " + a + "\n"; > document.getElementById("p").firstChild.data = > "Actual Results (unshift):\n" + s; >} >function y() { > var a = [0,1,2,3,4]; > delete a[1]; > var s = " before: " + a + "\n"; > a.splice(0,0,"a","b"); > s += " after: " + a + "\n"; > document.getElementById("y").firstChild.data = > "Actual Results (splice):\n" + s; > >} > >x(); >y(); ></SCRIPT> ></BODY></HTML>
Updated•19 years ago
|
Attachment #203005 -
Attachment is obsolete: true
Comment 11•19 years ago
|
||
Comment 12•19 years ago
|
||
RCS file: /cvsroot/mozilla/js/tests/js1_5/Array/regress-315509-01.js,v done Checking in regress-315509-01.js; /cvsroot/mozilla/js/tests/js1_5/Array/regress-315509-01.js,v <-- regress-315509-01.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/js/tests/js1_5/Array/regress-315509-02.js,v done Checking in regress-315509-02.js; /cvsroot/mozilla/js/tests/js1_5/Array/regress-315509-02.js,v <-- regress-315509-02.js initial revision: 1.1 done verified pass in 2005-11-13/Firefox trunk/WinXp
Status: RESOLVED → VERIFIED
Flags: testcase? → testcase+
Comment 13•19 years ago
|
||
*** Bug 317583 has been marked as a duplicate of this bug. ***
Summary: Crash: array_unshift doesn't handle holes properly [@ js_DeleteProperty] → Crash: array_unshift doesn't handle holes properly [@ js_DeleteProperty -array_unshift]
Summary: Crash: array_unshift doesn't handle holes properly [@ js_DeleteProperty -array_unshift] → Crash: array_unshift doesn't handle holes properly [@ js_DeleteProperty - array_unshift]
Comment 14•19 years ago
|
||
For drivers: in Czech republic we have this crash on one of 10 the most visited sites (duplicated bug 317583) and we already have some negative feedback from users testing RC3.
Flags: blocking1.8.0.1?
Keywords: regression
Comment 15•19 years ago
|
||
According to http://talkback-public.mozilla.org/reports/firefox/FF15rc3/FF15rc3-topcrashers.html, js_DeleteProperty is the #46 crash with "only" 0.33% of all crashes.
Comment 16•19 years ago
|
||
*** Bug 318415 has been marked as a duplicate of this bug. ***
Comment 17•19 years ago
|
||
(In reply to comment #14) > For drivers: in Czech republic we have this crash on one of 10 the most visited > sites (duplicated bug 317583) and we already have some negative feedback from > users testing RC3. For the record, this should have been fixed for RC3. I take blame for not nominating it. I thought it would be minus'ed, and that I'd have to override a bunch of people and make them feel stepped on. Clearly I should have nominated it, argued for it, and if necessary overridden any objections. Sorry, /be
Comment 18•19 years ago
|
||
if the fix can be included in the next firefox security and stability release, that would be good I think. This bug makes crash noxtrum.com, the new beta search engine from TelCo giant Telefonica ("yellow pages" in Spain and SouthAmerica). As soon as their search services goes out of beta it will be used on the whole Terra network and the crash will be very visible to the general public.
Comment 19•19 years ago
|
||
This is a no-brainer for both the patch release and the next planned branch-based product release. /be
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1+
Comment 20•19 years ago
|
||
*** Bug 318548 has been marked as a duplicate of this bug. ***
Comment 21•19 years ago
|
||
*** Bug 318541 has been marked as a duplicate of this bug. ***
Comment 22•19 years ago
|
||
*** Bug 318655 has been marked as a duplicate of this bug. ***
Comment 23•19 years ago
|
||
Comment on attachment 202209 [details] [diff] [review] patch This bug is a 1.5 topcrash and we are fixing it in 1.8.0.1. Nominating so someone else can plus. /be
Attachment #202209 -
Flags: approval1.8.1?
Attachment #202209 -
Flags: approval1.8.0.1?
Comment 24•19 years ago
|
||
Comment on attachment 202209 [details] [diff] [review] patch a=dveditz for drivers
Attachment #202209 -
Flags: approval1.8.1?
Attachment #202209 -
Flags: approval1.8.1+
Attachment #202209 -
Flags: approval1.8.0.1?
Attachment #202209 -
Flags: approval1.8.0.1+
Comment 26•19 years ago
|
||
verified fixed on the branch using https://bugzilla.mozilla.org/attachment.cgi?id=202208. Using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060110 Firefox/1.5.0.1. Adding verified1.8.0.1 keyword.
Keywords: verified1.8.0.1
Comment 27•19 years ago
|
||
*** Bug 323038 has been marked as a duplicate of this bug. ***
Updated•18 years ago
|
Keywords: fixed1.8.1
Updated•18 years ago
|
Keywords: fixed1.8.1
Updated•13 years ago
|
Crash Signature: [@ js_DeleteProperty - array_unshift]
You need to log in
before you can comment on or make changes to this bug.
Description
•