599166 - potential overflow in xml_setNamespace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Fast native xml_setNamespace can write to vp[2] when argc == 0

Here is the relevant code:

static JSBool
xml_setNamespace(JSContext *cx, uintN argc, jsval *vp)
{
    JSObject *qn;
    JSObject *ns;
    jsval qnargv[2];
    JSXML *nsowner;

    NON_LIST_XML_METHOD_PROLOG;
    if (!JSXML_HAS_NAME(xml))
        return JS_TRUE;

    xml = CHECK_COPY_ON_WRITE(cx, xml, obj);
    if (!xml)
        return JS_FALSE;

    ns = js_ConstructObject(cx, &js_NamespaceClass, NULL, obj,
                            argc == 0 ? 0 : 1, Valueify(vp + 2));
    if (!ns)
        return JS_FALSE;
    vp[0] = OBJECT_TO_JSVAL(ns);
    ns->setNamespaceDeclared(JSVAL_TRUE);

    qnargv[0] = vp[2] = OBJECT_TO_JSVAL(ns);
    qnargv[1] = OBJECT_TO_JSVAL(xml->name);

Comment 1

[Andreas Gal :gal]

Potentially exploitable if we reach the end of the 2MB stack chunk (free memory write).

Comment 2

[Andreas Gal :gal]

Looks like vp[2] is used here for rooting. That's no longer necessary (conservative stack scanner!).

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Daniel Veditz [:dveditz]]

We'll take it on the branches as soon as it's ready to go for trunk. If it makes next week's code-freeze that's great, otherwise next time.

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/76b7f615bab9

Comment 6

[Andreas Gal :gal]

sayrer, do you mind landing this on the branches? should be trivial. I don't have decent connectivity here

Comment 7

[Reed Loden [:reed]]

(In reply to comment #6)
> sayrer, do you mind landing this on the branches? should be trivial. I don't
> have decent connectivity here

It doesn't have approval yet, so it can't land on the branches anyway.

Comment 8

[Andreas Gal :gal]

reed, I would appreciate if you don't reassign bugs without consent of the assigning or receiving party.

clegnitto emailed me about this bug and I was reacting to that email, trying to make sure this bug is being handled in my absence.

If sayrer wants me to hold onto a bug while I am on PTO and a release is pending, he will assign it back to me.

Please do not change the assignee of this bug again without consent from sayrer, a JS peer or a release driver.

Comment 9

[Reed Loden [:reed]]

The bug assignee is always the patch writer. That's how things work. You don't get a choice in the matter. There are other ways to track when a bug needs work, and those ways should be used rather than misusing the assignee field.

Comment 10

[Andreas Gal :gal]

Please point me to the policy that supports your claim that contributors cannot hand off bugs in their own module to each other. I am highly doubtful that such a policy exists.

We do, however, have a binding Bugzilla Etiquette:

https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

"2. Changing Fields

    No messing with other people's bugs. Unless you are the bug assignee, or have some say over the use of their time, never change the Priority or Target Milestone fields. If in doubt, do not change the fields of bugs you do not own - add a comment instead, suggesting the change."

You are not the assignee, nor do you have say over the use of my time, so please keep your hands of my bugs.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 478107 [details] [diff] [review]
patch

So is this the right branch patch, or is reed just guessing with the approval requests? One interpretation of Andreas reassigning without requesting approval is that some porting work needs to be done.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 478107 [details] [diff] [review]
patch

oops, didn't mean to approve yet. It's certainly approvable if this is the right branch patch.

Comment 13

[Andreas Gal :gal]

#11: From a quick check the patch needs a trivial merge. I will be back Tuesday night, if sayrer didn't get to this until then I will do it.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 478107 [details] [diff] [review]
patch

Approved for 1.9.2.13 and 1.9.1.16, a=dveditz for release-drivers

Comment 15

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/def535668c41

Comment 16

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b0216c5e5f4a

Comment 17

[Mike Hommey [:glandium]]

Considering comment 2, and the fact that if I'm not mistaken, conservative stack scanner is only available on m-c, is it really the right fix for branches ?

Comment 18

[LegNeato]

Will leaving in this (potentially incomplete) fix cause crashing/correctness issues for the release?

Comment 19

[Al Billings [:abillings - ex-MoCo]]

I know that Reed is wanting this verified for branches but I see not STR or testcase available. How exactly is QA supposed to verify that this one line change is fixed?

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

This is difficult to verify without valgrind. The patch is trivially correct, though.

(In reply to comment #17)
> Considering comment 2, and the fact that if I'm not mistaken, conservative
> stack scanner is only available on m-c, is it really the right fix for branches
> ?

We actually root ns in vp[0] (which is guaranteed to exist) right after its creation. Its assignment into vp[2] was extra rooting that had the additional property of being wrong some of the time. So this doesn't introduce any GC hazards and it fixes a potential out-of-bounds write.

Comment 21

[Joshua Mitchell (Inactive)]

Issue is resolved - clearing old keywords - qa-wanted clean-up