370488 - GC hazard in GetProperty from jsxml.c

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

GetProperty from jsxml.c when called for XML list converts the propety value into  qname and without rooting it calls self recursively for list's elements passing the original value as id. This can be exploited as the following example demonstrates:

~/m/trunk/mozilla/js/src $ cat ~/s/y.js
var list = <><a><c/></a></>;
var result = list[{toString: f, c: 0}];

function f() {
  this.c++;
  if (this.c == 1) {
    for (var i = 0; i != 10000; ++i) {
      new QName();
    }
    return "c";    
  } else {
    gc();
  }
  return "c";    
}

gc();
~/m/trunk/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/s/y.js
before 600080, after 64624, break 09b6d000
Assertion failure: offsetInArena < GC_THINGS_SIZE, at jsgc.c:494
trace/breakpoint trap

Here on the first invocation f as toString implementation makes sure that the current free list for qnames is emptied and new arenas are allocated. On the second invication it runs GC. That collects the previously allocated qnames including the one that exists in GetProperty frame and free their GC arenas. Then uppermost GetProperty on return put the qname pointed to freed memory into live xml list object as its target property. That triggers the crash during the second GC.

Comment 1

[Igor Bukanov]

I checked jsxml.c and it seems only GetProperty has this type of problems. In other cases the code is either roots the result of ToXMLname or calls that under js_EnterLocalRootScope.

Yet there is another worry. ToXMLname for function qnames may end up constructing a new atom for function name. That unrooted atom is passed to js_GetProperty/js_SetProperty. As far as I can see it is not possible to trigger a full GC during those calls given that a new atom means that the property does not exist. Yet, for example, prototype chain manipulations may alter that no-full-GC property.       

Comment 2

[Igor Bukanov]

This is a regression introduced in bug 336921. Prior to it unrooted nameqn would never be stored in targetprop.

Comment 3

[Igor Bukanov]

Attachment: Fix v1

The fix refactors GetProperty to chose code branches based on the type of qname, not the type of the xml objects. In addition the case of XML object is implemented through helper function that is also used in the body of the list loop avoiding all that extra overhead of intermediate list objects and multiple ToXMLName calls.

Then the fix uses explicit temporary roots for nameqn and listobj. Although after the refactoring the code is safe even without any rooting due to the presence weak roots, weak roots are too fragile to rely on them.

Comment 4

[Igor Bukanov]

Here is for references the refactored implementation of GetProperty as it is not very redable in the patch form:

/* ECMA-357 9.1.1.1 XML [[Get]] and 9.2.1.1 XMLList [[Get]]. */
static JSBool
GetProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
{
    JSXML *xml, *list, *kid;
    uint32 index;
    JSObject *kidobj;
    JSXMLQName *nameqn;
    jsid funid;
    enum {
        NAME = 0,
        LIST = 1
    };
    JSObject *roots[2];
    JSTempValueRooter tvr;
    JSBool attributes;
    JSXMLArrayCursor cursor;

    xml = (JSXML *) JS_GetInstancePrivate(cx, obj, &js_XMLClass, NULL);
    if (!xml)
        return JS_TRUE;

    if (js_IdIsIndex(id, &index)) {
        if (xml->xml_class != JSXML_CLASS_LIST) {
            obj = ToXMLList(cx, OBJECT_TO_JSVAL(obj));
            if (!obj)
                return JS_FALSE;
            xml = (JSXML *) JS_GetPrivate(cx, obj);
        }

        /*
         * ECMA-357 9.2.1.1 starts here.
         *
         * Erratum: 9.2 is not completely clear that indexed properties
         * correspond to kids, but that's what it seems to say, and it's
         * what any sane user would want.
         */
        if (index < xml->xml_kids.length) {
            kid = XMLARRAY_MEMBER(&xml->xml_kids, index, JSXML);
            if (!kid) {
                *vp = JSVAL_VOID;
                return JS_TRUE;
            }
            kidobj = js_GetXMLObject(cx, kid);
            if (!kidobj)
                return JS_FALSE;

            *vp = OBJECT_TO_JSVAL(kidobj);
        } else {
            *vp = JSVAL_VOID;
        }
        return JS_TRUE;
    }

    /* ECMA-357 9.2.1.1/9.1.1.1 qname case. */

    nameqn = ToXMLName(cx, id, &funid);
    if (!nameqn)
        return JS_FALSE;
    roots[NAME] = nameqn->object;

    if (funid)
        return GetFunction(cx, obj, xml, funid, vp);

    roots[NAME] = nameqn->object;
    roots[LIST] = NULL;
    JS_PUSH_TEMP_ROOT_OBJECTS(cx, JS_ARRAY_LENGTH(roots), roots, &tvr);

    attributes = (OBJ_GET_CLASS(cx, nameqn->object) == &js_AttributeNameClass);

    roots[LIST] = js_NewXMLObject(cx, JSXML_CLASS_LIST);
    if (!roots[LIST]) {
        list = NULL;
        goto out;
    }
    list = (JSXML *) JS_GetPrivate(cx, roots[LIST]);

    if (xml->xml_class == JSXML_CLASS_LIST) {
        XMLArrayCursorInit(&cursor, &xml->xml_kids);
        while ((kid = (JSXML *) XMLArrayCursorNext(&cursor)) != NULL) {
            if (kid->xml_class == JSXML_CLASS_ELEMENT &&
                !GetPropertyHelper(cx, list, kid, nameqn, attributes)) {
                list = NULL;
                break;
            }
        }
        XMLArrayCursorFinish(&cursor);
    } else {
        if (!GetPropertyHelper(cx, list, xml, nameqn, attributes))
            list = NULL;
    }

  out:
    JS_POP_TEMP_ROOT(cx, &tvr);
    if (!list)
        return JS_FALSE;

    /*
     * Erratum: ECMA-357 9.1.1.1 misses that [[Append]] sets the given list's
     * [[TargetProperty]] to the property that is being appended. This means
     * that any use of the internal [[Get]] property returns a list which,
     * when used by e.g. [[Insert]] duplicates the last element matched by id.
     * See bug 336921.
     */
    list->xml_target = xml;
    list->xml_targetprop = nameqn;
    *vp = OBJECT_TO_JSVAL(roots[LIST]);
    return JS_TRUE;
}

Comment 5

[Igor Bukanov]

Attachment: Fix v2

The previous code contained double-initialization of roots. here is a fixed version. The code for GetProperty is here:

/* ECMA-357 9.1.1.1 XML [[Get]] and 9.2.1.1 XMLList [[Get]]. */
static JSBool
GetProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
{
    JSXML *xml, *list, *kid;
    uint32 index;
    JSObject *kidobj;
    JSXMLQName *nameqn;
    jsid funid;
    enum {
        NAME = 0,
        LIST = 1
    };
    JSObject *roots[2];
    JSTempValueRooter tvr;
    JSBool attributes;
    JSXMLArrayCursor cursor;

    xml = (JSXML *) JS_GetInstancePrivate(cx, obj, &js_XMLClass, NULL);
    if (!xml)
        return JS_TRUE;

    if (js_IdIsIndex(id, &index)) {
        if (xml->xml_class != JSXML_CLASS_LIST) {
            obj = ToXMLList(cx, OBJECT_TO_JSVAL(obj));
            if (!obj)
                return JS_FALSE;
            xml = (JSXML *) JS_GetPrivate(cx, obj);
        }

        /*
         * ECMA-357 9.2.1.1 starts here.
         *
         * Erratum: 9.2 is not completely clear that indexed properties
         * correspond to kids, but that's what it seems to say, and it's
         * what any sane user would want.
         */
        if (index < xml->xml_kids.length) {
            kid = XMLARRAY_MEMBER(&xml->xml_kids, index, JSXML);
            if (!kid) {
                *vp = JSVAL_VOID;
                return JS_TRUE;
            }
            kidobj = js_GetXMLObject(cx, kid);
            if (!kidobj)
                return JS_FALSE;

            *vp = OBJECT_TO_JSVAL(kidobj);
        } else {
            *vp = JSVAL_VOID;
        }
        return JS_TRUE;
    }

    /* ECMA-357 9.2.1.1/9.1.1.1 qname case. */

    nameqn = ToXMLName(cx, id, &funid);
    if (!nameqn)
        return JS_FALSE;
    roots[NAME] = nameqn->object;

    if (funid)
        return GetFunction(cx, obj, xml, funid, vp);

    roots[NAME] = nameqn->object;
    roots[LIST] = NULL;
    JS_PUSH_TEMP_ROOT_OBJECTS(cx, JS_ARRAY_LENGTH(roots), roots, &tvr);

    attributes = (OBJ_GET_CLASS(cx, nameqn->object) == &js_AttributeNameClass);

    roots[LIST] = js_NewXMLObject(cx, JSXML_CLASS_LIST);
    if (!roots[LIST]) {
        list = NULL;
        goto out;
    }
    list = (JSXML *) JS_GetPrivate(cx, roots[LIST]);

    if (xml->xml_class == JSXML_CLASS_LIST) {
        XMLArrayCursorInit(&cursor, &xml->xml_kids);
        while ((kid = (JSXML *) XMLArrayCursorNext(&cursor)) != NULL) {
            if (kid->xml_class == JSXML_CLASS_ELEMENT &&
                !GetPropertyHelper(cx, list, kid, nameqn, attributes)) {
                list = NULL;
                break;
            }
        }
        XMLArrayCursorFinish(&cursor);
    } else {
        if (!GetPropertyHelper(cx, list, xml, nameqn, attributes))
            list = NULL;
    }

  out:
    JS_POP_TEMP_ROOT(cx, &tvr);
    if (!list)
        return JS_FALSE;

    /*
     * Erratum: ECMA-357 9.1.1.1 misses that [[Append]] sets the given list's
     * [[TargetProperty]] to the property that is being appended. This means
     * that any use of the internal [[Get]] property returns a list which,
     * when used by e.g. [[Insert]] duplicates the last element matched by id.
     * See bug 336921.
     */
    list->xml_target = xml;
    list->xml_targetprop = nameqn;
    *vp = OBJECT_TO_JSVAL(roots[LIST]);
    return JS_TRUE;
}

Comment 6

[Igor Bukanov]

Attachment: Fix v3

The new version avoids creation of intermediate list when GetProperty is called with index for the non-list case. Here is GetProperty again:

static JSBool
GetProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
{
    JSXML *xml, *list, *kid;
    uint32 index;
    JSObject *kidobj;
    JSXMLQName *nameqn;
    jsid funid;
    enum {
        NAME = 0,
        LIST = 1
    };
    JSObject *roots[2];
    JSTempValueRooter tvr;
    JSBool attributes;
    JSXMLArrayCursor cursor;

    xml = (JSXML *) JS_GetInstancePrivate(cx, obj, &js_XMLClass, NULL);
    if (!xml)
        return JS_TRUE;

    if (js_IdIsIndex(id, &index)) {
        if (xml->xml_class != JSXML_CLASS_LIST) {
            *vp = (index == 0) ? OBJECT_TO_JSVAL(obj) : JSVAL_VOID; 
        } else {
            /*
             * ECMA-357 9.2.1.1 starts here.
             *
             * Erratum: 9.2 is not completely clear that indexed properties
             * correspond to kids, but that's what it seems to say, and it's
             * what any sane user would want.
             */
            if (index < xml->xml_kids.length) {
                kid = XMLARRAY_MEMBER(&xml->xml_kids, index, JSXML);
                if (!kid) {
                    *vp = JSVAL_VOID;
                    return JS_TRUE;
                }
                kidobj = js_GetXMLObject(cx, kid);
                if (!kidobj)
                    return JS_FALSE;
                
                *vp = OBJECT_TO_JSVAL(kidobj);
            } else {
                *vp = JSVAL_VOID;
            }
        }
        return JS_TRUE;
    }

    /* ECMA-357 9.2.1.1/9.1.1.1 qname case. */

    nameqn = ToXMLName(cx, id, &funid);
    if (!nameqn)
        return JS_FALSE;
    if (funid)
        return GetFunction(cx, obj, xml, funid, vp);

    roots[NAME] = nameqn->object;
    roots[LIST] = NULL;
    JS_PUSH_TEMP_ROOT_OBJECTS(cx, JS_ARRAY_LENGTH(roots), roots, &tvr);

    attributes = (OBJ_GET_CLASS(cx, nameqn->object) == &js_AttributeNameClass);

    roots[LIST] = js_NewXMLObject(cx, JSXML_CLASS_LIST);
    if (!roots[LIST]) {
        list = NULL;
        goto out;
    }
    list = (JSXML *) JS_GetPrivate(cx, roots[LIST]);

    if (xml->xml_class == JSXML_CLASS_LIST) {
        XMLArrayCursorInit(&cursor, &xml->xml_kids);
        while ((kid = (JSXML *) XMLArrayCursorNext(&cursor)) != NULL) {
            if (kid->xml_class == JSXML_CLASS_ELEMENT &&
                !GetNamedProperty(cx, kid, nameqn, attributes, list)) {
                list = NULL;
                break;
            }
        }
        XMLArrayCursorFinish(&cursor);
    } else {
        if (!GetNamedProperty(cx, xml, nameqn, attributes, list))
            list = NULL;
    }

  out:
    JS_POP_TEMP_ROOT(cx, &tvr);
    if (!list)
        return JS_FALSE;

    /*
     * Erratum: ECMA-357 9.1.1.1 misses that [[Append]] sets the given list's
     * [[TargetProperty]] to the property that is being appended. This means
     * that any use of the internal [[Get]] property returns a list which,
     * when used by e.g. [[Insert]] duplicates the last element matched by id.
     * See bug 336921.
     */
    list->xml_target = xml;
    list->xml_targetprop = nameqn;
    *vp = OBJECT_TO_JSVAL(roots[LIST]);
    return JS_TRUE;
}

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 255259 [details] [diff] [review]
Fix v3

>@@ -550,14 +551,18 @@ JS_STATIC_ASSERT(sizeof(JSTempValueUnion
>  * alternatives to JS_PUSH_TEMP_ROOT_GCTHING for JSObject and JSString. They
>  * also provide a simple way to get a single pointer to rooted JSObject or
>  * JSString via JS_PUSH_TEMP_ROOT_(OBJECT|STRTING)(cx, NULL, &tvr). Then
>  * &tvr.u.object or tvr.u.string gives the necessary pointer, which puns
>  * tvr.u.value safely because JSObject * and JSString * are GC-things and, as
>  * such, their tag bits are all zeroes.
>  *
>+ * Similar JS_PUSH_TEMP_ROOT_OBJECTS is a type safe alternative to
>+ * JS_PUSH_TEMP_ROOT too root an array of objects and it works for the same
>+ * reasons.

s/too/to/

Wonder if it wouldn't be better to use the jsval *array member, casting. Nah, never mind.

>+    ok = JS_TRUE;

Blank line here would be nice.

>+  out:
>+    XMLArrayCursorFinish(&cursor);
>+    return ok;
>+}
>+
> /* ECMA-357 9.1.1.1 XML [[Get]] and 9.2.1.1 XMLList [[Get]]. */
> static JSBool
> GetProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
> {
[snip...]
>+    /* ECMA-357 9.2.1.1/9.1.1.1 qname case. */
> 

Maybe this blank line is overkill for a one-line comment. Could use a major comment?

>+    roots[LIST] = js_NewXMLObject(cx, JSXML_CLASS_LIST);
>+    if (!roots[LIST]) {
>+        list = NULL;
>+        goto out;

With just one goto out, tempting to use else clause and over-indent the guts by one level. Man, I'm fussy today. Whatever you prefer ;-).

Thanks for fixing this. I should have refactored the spec when I wrote this.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> >+ * Similar JS_PUSH_TEMP_ROOT_OBJECTS is a type safe alternative to
> >+ * JS_PUSH_TEMP_ROOT too root an array of objects and it works for the same
> >+ * reasons.
> 
> s/too/to/
> 
> Wonder if it wouldn't be better to use the jsval *array member, casting. Nah,
> never mind.

Oh, but this patch does rely on tvr->u.array punning tvr->u.objects, so I still wonder whether objects is necessary. It seems odd to use it to avoid a cast, but imply that the punning is safe. I'd rather see a cast when punning instead of a union arm overlaying another arm, but I admit it's a style (and very minor source line bloat) issue.

/be

Comment 9

[Igor Bukanov]

Attachment: Fix v4

The new version uses JS_PUSH_TEMP_ROOT to avoid touching jscntxt.h and replaces goto with if.

Comment 10

[Igor Bukanov]

Here again for references GetProperty implementation:

/* ECMA-357 9.1.1.1 XML [[Get]] and 9.2.1.1 XMLList [[Get]]. */
static JSBool
GetProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
{
    JSXML *xml, *list, *kid;
    uint32 index;
    JSObject *kidobj, *listobj;
    JSXMLQName *nameqn;
    jsid funid;
    jsval roots[2];
    JSTempValueRooter tvr;
    JSBool attributes;
    JSXMLArrayCursor cursor;

    xml = (JSXML *) JS_GetInstancePrivate(cx, obj, &js_XMLClass, NULL);
    if (!xml)
        return JS_TRUE;

    if (js_IdIsIndex(id, &index)) {
        if (xml->xml_class != JSXML_CLASS_LIST) {
            *vp = (index == 0) ? OBJECT_TO_JSVAL(obj) : JSVAL_VOID;
        } else {
            /*
             * ECMA-357 9.2.1.1 starts here.
             *
             * Erratum: 9.2 is not completely clear that indexed properties
             * correspond to kids, but that's what it seems to say, and it's
             * what any sane user would want.
             */
            if (index < xml->xml_kids.length) {
                kid = XMLARRAY_MEMBER(&xml->xml_kids, index, JSXML);
                if (!kid) {
                    *vp = JSVAL_VOID;
                    return JS_TRUE;
                }
                kidobj = js_GetXMLObject(cx, kid);
                if (!kidobj)
                    return JS_FALSE;

                *vp = OBJECT_TO_JSVAL(kidobj);
            } else {
                *vp = JSVAL_VOID;
            }
        }
        return JS_TRUE;
    }

    /*
     * ECMA-357 9.2.1.1/9.1.1.1 qname case.
     */
    nameqn = ToXMLName(cx, id, &funid);
    if (!nameqn)
        return JS_FALSE;
    if (funid)
        return GetFunction(cx, obj, xml, funid, vp);

    roots[0] = OBJECT_TO_JSVAL(nameqn->object);
    JS_PUSH_TEMP_ROOT(cx, 1, roots, &tvr);

    listobj = js_NewXMLObject(cx, JSXML_CLASS_LIST);
    if (listobj) {
        roots[1] = OBJECT_TO_JSVAL(listobj);
        tvr.count++;
        
        list = (JSXML *) JS_GetPrivate(cx, listobj);
        attributes = (OBJ_GET_CLASS(cx, nameqn->object) ==
                      &js_AttributeNameClass);

        if (xml->xml_class == JSXML_CLASS_LIST) {
            XMLArrayCursorInit(&cursor, &xml->xml_kids);
            while ((kid = (JSXML *) XMLArrayCursorNext(&cursor)) != NULL) {
                if (kid->xml_class == JSXML_CLASS_ELEMENT &&
                    !GetNamedProperty(cx, kid, nameqn, attributes, list)) {
                    listobj = NULL;
                    break;
                }
            }
            XMLArrayCursorFinish(&cursor);
        } else {
            if (!GetNamedProperty(cx, xml, nameqn, attributes, list))
                listobj = NULL;
        }

        /*
         * Erratum: ECMA-357 9.1.1.1 misses that [[Append]] sets the given
         * list's [[TargetProperty]] to the property that is being appended.
         * This means that any use of the internal [[Get]] property returns
         * a list which, when used by e.g. [[Insert]] duplicates the last
         * element matched by id.
         * See bug 336921.
         */
        list->xml_target = xml;
        list->xml_targetprop = nameqn;
        *vp = OBJECT_TO_JSVAL(listobj);
    }

    JS_POP_TEMP_ROOT(cx, &tvr);
    return listobj != NULL;
}

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 255316 [details] [diff] [review]
Fix v4

Looks good, r=me. Thanks,

/be

Comment 12

[Igor Bukanov]

I committed the patch from comment 10 to the trunk:
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.141; previous revision: 3.140
done

Comment 13

[Bob Clary [:bc] (inactive)]

Attachment: e4x/GC/regress-370488.js

I added this locally on 2/17 but forgot to record it here.

Comment 14

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 20070226 windows/mac*/linux.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 255316 [details] [diff] [review]
Fix v4

Would like a second review for a non-trivial js-engine patch on the branch.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 255316 [details] [diff] [review]
Fix v4

approved for 1.8.0.12 and 1.8.1.4, a=dveditz

Comment 17

[Igor Bukanov]

I committed the patch from comment 9 to MOZILLA_1_8_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.60; previous revision: 3.50.2.59
done

Comment 18

[Igor Bukanov]

Attachment: Fix v4 for 1.8.0

1.8.0 version of the patch required hand-merging due to trailing whitespace that exists on 1.8.0 and JS_LocalScope versus js_LocalScope usages.

Comment 19

[Igor Bukanov]

Attachment: diff between trunk and 1.8.0 versions of v4

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 260647 [details] [diff] [review]
Fix v4 for 1.8.0

Does lack of js_LeaveLocalRootScopeWithResult on the 1.8.0 branch mean there's a GC hazard propagating listobj out?

/be

Comment 21

[Igor Bukanov]

(In reply to comment #20)
> Does lack of js_LeaveLocalRootScopeWithResult on the 1.8.0 branch mean there's
> a GC hazard propagating listobj out?

I do not thing so. On the branch GetProperty is called either already inside local roots or with rooted-via-native-call rval. So js_LeaveLocalRootScopeWithResult was not necessary for GC-safty. 

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 260647 [details] [diff] [review]
Fix v4 for 1.8.0

approved for 1.8.0.12, a=dveditz for release-drivers

Comment 23

[Igor Bukanov]

Attachment: Fix v4 for 1.8.0 updated

The commit for bug 366975 affected this patch as it changed the text in comments this patch removes. So here is an update.

Comment 24

[Igor Bukanov]

Attachment: Diff between previously approved and the new version for 1.8.0

Here is a proof of triviality of the update as bugzilla diff can not show the difference.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 261106 [details] [diff] [review]
Fix v4 for 1.8.0 updated

approved for 1.8.0.12, a=dveditz for release-drivers

Comment 26

[Igor Bukanov]

I committed the patch from comment 23 to MOZILLA_1_8_0_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.15.2.36; previous revision: 3.50.2.15.2.35
done

Comment 27

[Bob Clary [:bc] (inactive)]

verified fixed windows, macppc, linux 1.8.0, 1.8.1 shell 20070417