375406 - Crash [@ PutProperty] setting <a/>.attribute('')[0]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> <a/>.attribute('')[0] = 1;
Bus error

Tested in debug js shell on Mac.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Crashes latest opt js shell as well:

gary-kwongs-mac-mini:lithium gkwong$ ./js-opt 
js> <a/>.attribute('')[0] = 1;
Bus error

=======

gary-kwongs-mac-mini:lithium gkwong$ ./js-debug 
js> <a/>.attribute('')[0] = 1;
Bus error


Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008040204 Minefield/3.0pre

Thus it definitely crashes latest Minefield as well.

http://crash-stats.mozilla.com/report/index/a74be521-00e1-11dd-a574-001a4bd43e5c

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This should be a regression of bug 311071.

Seems to work as expected with cvs js shell checkout at 2005-10-04 13:24 PDT
Crashes with cvs js shell checkout at 2005-10-04 13:26 PDT

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-10-04+13%3A24%3A00&maxdate=2005-10-04+13%3A26%3A00&cvsroot=%2Fcvsroot

Bonsai message:

Revise XML vs. HTML comment/CDATA handling to require JSOPTION_XML for former (311071, r/sr=mrbkap/shaver, a=dbaron).

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: crash log

This crashes TM tip (without -j) too.

Comment 4

[Igor Bukanov]

The bug comes fro the following lines of PutProperty, http://mxr.mozilla.org/mozilla-central/source/js/src/jsxml.cpp#4275 :

4275             id = OBJECT_TO_JSVAL(nameobj);
...
4282                 ok = PutProperty(cx, parentobj, id, vp);
...
4287                 ok = GetProperty(cx, parentobj, id, vp);
...
4290                 attr = (JSXML *) JS_GetPrivate(cx, JSVAL_TO_OBJECT(*vp));
...
4293                 xml->xml_kids.vector[i] = attr->xml_kids.vector[0];

Here the code assumes that if PutProperty succeeds, then it would unconditionally adds an attribute to parentobj so GetProperty would return non-empty list. 

This assumption is wrong as nameobj can have a local name that does not pass IsXMLName() test. For example, in the example from the comment 0 the localname of the nameobj is an empty string. When this happens, PutProperty does nothing so the following GetProperty returns an empty xml list.

I will check if this is a bug in the specs but a simplest fix is to assign xml_kids only if attrs has element.

As regarding the severity, the bug is really null pointer read.

Comment 5

[Igor Bukanov]

This is a bug in E4X specs. From ECMA-357 v2, 9.2.1.2 [[Put]] (P, V) (Put for XMLList):

...
e. If x[i].[[Class]] == "attribute"
       i. Let z = ToAttributeName(x[i].[[Name]])
      ii. Call the [[Put]] method of x[i].[[Parent]] with arguments z and V
     iii. Let attr be the result of calling [[Get]] on x[i].[[Parent]] with argument z
     iv. Let x[i] = attr[0]

That is, the specs also assumes that [[Put]] always adds or sets argument so [[Get]] returns non-empty list. Yet 9.1.1.2 [[Put]] (P, V) (Put for XML) dictates:

6. If Type(n) is AttributeName
    a. Call the function isXMLName (section 13.1.2.1) with argument n.[[Name]] and if the result is false,
        return

Comment 6

[Igor Bukanov]

Attachment: v1

The patch adds the array length check before doing the assignment. As a consequence of the fix the following code:

var list = <a/>.attribute('\0');
list[0] = 1;
print(list.length());

would print 1 even if the element <a/> is not mutated and still contains 0 attributes. This comes from code in PutProperty (it follows the specs) that would add a temporary attribute element to the list when the index passed to PutProperty exceeds the length of the list. The assumption here is that the temporary will be later overwritten by the right thing.

But such discrepancy between list.length() and the real number of attributes in the underlying xml is already exists in the code. First, any OOM-error after adding the temporary would leave the list in the inconsistent state. Second, and more easy to trigger, any error thrown during ToString conversion from a custom toString implementation of the value passed to PutProperty, would also leave the list with a bogus extra element. Thus the patch does not add anything new. 

AFAICS the only consequence of the discrepancy is peculiar semantics of such lists.

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> But such discrepancy between list.length() and the real number of attributes in
> the underlying xml is already exists in the code.

I should not worry about that. The state of the list in such case is no different from the case of:

var xml = <a b='1'/>;
var list = xml.attribute('b');
delete xml.@b;

After this fragment list.length() is still 1 and contains an attribute element pointing to no longer existing b in xml.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 369166 [details] [diff] [review]
v1

E4X was helpful in restarting Ecma TC39 work on general evolution of the JS standard -- but it came at a price. :-/

/be

Comment 9

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/8cd63b7eb177

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/8cd63b7eb177

Comment 11

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/51f15dbfb745

Comment 12

[Bob Clary [:bc] (inactive)]

e4x/Regress/regress-375406.js	
http://hg.mozilla.org/tracemonkey/rev/5e5203396ac8

Comment 13

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 14

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/e4x/Regress/regress-375406.js,v  <--  regress-375406.js
initial revision: 1.1