Closed Bug 315509 Opened 16 years ago Closed 16 years ago

Crash: array_unshift doesn't handle holes properly [@ js_DeleteProperty - array_unshift]

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

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)

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
Attached file testcase 2 - crash
Attached patch patchSplinter Review
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+
Assignee: general → moz_bug_r_a4
Flags: testcase?
Fix checked into trunk. Nominating for 1.8.1 since we probably would want this sort of fix.
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: blocking1.8.1?
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
*** Bug 316208 has been marked as a duplicate of this bug. ***
Keywords: crash
Summary: Crash: array_unshift doesn't handle holes properly → Crash: array_unshift doesn't handle holes properly [@ js_DeleteProperty]
Any chance this can land on 1.8.0 so that we don't have to wait for 1.8.1?
(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
Attached file Testcase for workaround using splice (obsolete) —
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>
Attachment #203005 - Attachment is obsolete: true
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+
*** 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]
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
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.
*** Bug 318415 has been marked as a duplicate of this bug. ***
(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
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.
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+
*** Bug 318548 has been marked as a duplicate of this bug. ***
*** Bug 318541 has been marked as a duplicate of this bug. ***
*** Bug 318655 has been marked as a duplicate of this bug. ***
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 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+
Fix checked into branches.
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
*** Bug 323038 has been marked as a duplicate of this bug. ***
v 2006-01-11 builds on windows/linux/mac
Keywords: fixed1.8.1
Crash Signature: [@ js_DeleteProperty - array_unshift]
You need to log in before you can comment on or make changes to this bug.