347330 - localrootscope in jsxml.c

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[georgi - hopefully not receiving bugspam]

localrootscope in jsxml.c

not sure this is a bug, so feel free to resolve as as invalid.

in jsxml.c js_EnterLocalRootScope and js_LeaveLocalRootScopeWithResult
seem balanced on purpose and some nasty bugs mention incorrectly rooted
things.

in jsxml.c ToXMLList
[1]        if (!js_EnterLocalRootScope(cx))
            return NULL;
        xml = ParseXMLSource(cx, str);
        if (!xml) {
            js_LeaveLocalRootScope(cx);
            return NULL;
        }
        length = JSXML_LENGTH(xml);
    }

    listobj = js_NewXMLObject(cx, JSXML_CLASS_LIST);
    if (listobj) {
        list = (JSXML *) JS_GetPrivate(cx, listobj);
        for (i = 0; i < length; i++) {
            kid = OrphanXMLChild(cx, xml, i);
            if (!kid)
[2]                return NULL;

so if [1] and [2] get hit (probably due to out of memory) root scope will
be not balanced because of the early return.


also in jsxml.c GetProperty "goto retry" potentially may disbalance the
rootscope.

so in case this is exploitable i may try to write a testcase.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

This looks like a bug, though I don't believe it's exploitable. In the worst case, this will result in a massive memory leak: every object allocated after the erroneous call will be reachable according to the GC.

I don't believe the goto retry in GetProperty is a problem. There is one goto in the function, which is before the local root scope in the else branch is entered, which means that we cannot goto retry after having pushed our own local root scope. Did I miss something? I'll patch the first issue in a jiffy.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Oh, how I pine for JSAutoLocalRootScope! ;-)

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 232182 [details] [diff] [review]
Fix

r=me.  E4X, yay.

/be

Comment 4

[Brendan Eich [:brendan]]

This is "just" a leak, not a dangling pointer vulnerability.

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Checked into trunk.

Comment 6

[georgi - hopefully not receiving bugspam]

(In reply to comment #1)
> 
> I don't believe the goto retry in GetProperty is a problem. There is one goto
> in the function, which is before the local root scope in the else branch is
> entered, which means that we cannot goto retry after having pushed our own
> local root scope. Did I miss something? I'll patch the first issue in a jiffy.
> 

you are right, i seem wrong on the second issue.

are there other unbalanced constructs that may lead to troubles?

Comment 7

[Mike Schroepfer]

Comment on attachment 232182 [details] [diff] [review]
Fix

a=schrep for drivers.

Comment 8

[Brendan Eich [:brendan]]

I checked the fix into the 1.8 branch.

/be