Last Comment Bug 599166 - potential overflow in xml_setNamespace
: potential overflow in xml_setNamespace
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla2.0b7
Assigned To: Andreas Gal :gal
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2010-09-23 16:21 PDT by Brian Hackett (:bhackett)
Modified: 2014-10-12 13:58 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (697 bytes, patch)
2010-09-23 16:32 PDT, Andreas Gal :gal
brendan: review+
dveditz: approval1.9.2.13+
dveditz: approval1.9.1.16+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2010-09-23 16:21:14 PDT
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;

    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);

    qnargv[0] = vp[2] = OBJECT_TO_JSVAL(ns);
    qnargv[1] = OBJECT_TO_JSVAL(xml->name);
Comment 1 Andreas Gal :gal 2010-09-23 16:28:07 PDT
Potentially exploitable if we reach the end of the 2MB stack chunk (free memory write).
Comment 2 Andreas Gal :gal 2010-09-23 16:31:32 PDT
Looks like vp[2] is used here for rooting. That's no longer necessary (conservative stack scanner!).
Comment 3 Andreas Gal :gal 2010-09-23 16:32:19 PDT
Created attachment 478107 [details] [diff] [review]
Comment 4 Daniel Veditz [:dveditz] 2010-09-24 10:28:16 PDT
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 6 Andreas Gal :gal 2010-11-06 01:37:21 PDT
sayrer, do you mind landing this on the branches? should be trivial. I don't have decent connectivity here
Comment 7 Reed Loden [:reed] (use needinfo?) 2010-11-06 02:19:37 PDT
(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 2010-11-06 03:00:56 PDT
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] (use needinfo?) 2010-11-06 03:13:08 PDT
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 2010-11-06 04:28:08 PDT
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:

"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] 2010-11-08 10:38:04 PST
Comment on attachment 478107 [details] [diff] [review]

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] 2010-11-08 10:40:15 PST
Comment on attachment 478107 [details] [diff] [review]

oops, didn't mean to approve yet. It's certainly approvable if this is the right branch patch.
Comment 13 Andreas Gal :gal 2010-11-08 11:07:43 PST
#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] 2010-11-10 10:39:35 PST
Comment on attachment 478107 [details] [diff] [review]

Approved for and, a=dveditz for release-drivers
Comment 15 Reed Loden [:reed] (use needinfo?) 2010-11-19 01:30:39 PST
Comment 16 Reed Loden [:reed] (use needinfo?) 2010-11-19 02:00:29 PST
Comment 17 Mike Hommey [:glandium] 2010-12-08 01:14:52 PST
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 christian 2010-12-08 14:17:50 PST
Will leaving in this (potentially incomplete) fix cause crashing/correctness issues for the release?
Comment 19 Al Billings [:abillings] 2010-12-08 16:04:33 PST
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) 2010-12-08 16:33:51 PST
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) 2014-10-12 13:58:51 PDT
Issue is resolved - clearing old keywords - qa-wanted clean-up

Note You need to log in before you can comment on or make changes to this bug.