Closed Bug 720079 Opened 13 years ago Closed 13 years ago

array.join("") is GC-hazardous


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox10 --- wontfix
firefox11 --- verified
firefox-esr10 11+ verified
status1.9.2 --- unaffected


(Reporter: Waldo, Assigned: Waldo)



(Whiteboard: [sg:critical][qa!])


(1 file)

/* Elements beyond the initialized length are 'undefined' and thus can be ignored. */
    const Value *beg = obj->getDenseArrayElements();
    const Value *end = beg + Min(length, obj->getDenseArrayInitializedLength());
    for (const Value *vp = beg; vp != end; ++vp) {
        if (!JS_CHECK_OPERATION_LIMIT(cx))
            return false;

        if (!vp->isMagic(JS_ARRAY_HOLE) && !vp->isNullOrUndefined()) {
            if (!ValueToStringBuffer(cx, *vp, sb))
                return false;

ValueToStringBuffer can invoke arbitrary user code, making the raw element iteration patently unsafe.  See this, for example (code which should include no random-looking doubles of the sort printed, obviously):

[jwalden@wheres-wally src]$ dbg/js
js> var arr = [1, 2, 3, 4, 5, 6, 7, { toString: function() { arr.length = 0; gc(); arr.foopy = 17; gc(); arr = null; gc(); var q = 0; while (q++ < 1e3) void {}; gc(); return "bwhahaha"; } }, 9, 10, 11, {}, 13, 14, 15, 16, { toString: function() { return 42; } }, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27]; arr.join("")
I posted a patch which happens to fix this in bug 720511.  There was a pre-existing spec compliance issue which could serve as a diversionary issue suitable for fixing in a non-hidden bug, so it seemed worth running with that so anyone watching for hidden-bug checkins won't notice the fix.
[Approval Request Comment]
Regression caused by: bug 587257
User impact if declined: Leaving this open should enable arbitrary code execution, with enough effort.

Testing completed (on m-c, etc.): The original test in this bug no longer spews valgrind warnings and the test in the patch (for the issue claimed to be the reason for the patch when it was posted) are what's been done thus far.

I'm happy to let this bake a bit, just feel like I should post this sooner rather than later in case there's a chance of getting it into the forthcoming Firefox release.

Risk to taking this patch (and alternatives if risky): Not particularly risky, as it mostly reverts things to slightly slower paths that are less dependent on exact characteristics of some of the methods at play.  Anyone who understands how our Array optimizations for dense arrays work should have a pretty easy time understand and verifying it.

As usual, I have no idea which trees are really open to fixes right now, this close to another release, so I'm requesting aurora/beta and will let branch triage sort things out.
Attachment #590948 - Flags: approval-mozilla-beta?
Attachment #590948 - Flags: approval-mozilla-aurora?
Comment on attachment 590948 [details] [diff] [review]
Patch from bug 720511, applies cleanly to aurora/beta both

[Triage Comment]
We're holding this bug to the standard of "would we chemspill FF10 for this issue?" Because it was internally found, we wouldn't. Approving for Aurora 11 but denying for Beta 10.
Attachment #590948 - Flags: approval-mozilla-beta?
Attachment #590948 - Flags: approval-mozilla-beta-
Attachment #590948 - Flags: approval-mozilla-aurora?
Attachment #590948 - Flags: approval-mozilla-aurora+
Pushed to aurora:

The change still links to the public bug, and I hand-waved a comment in it about finding a site which broke due to the bug, and discussing landing the change in aurora to fix it.  (It might have been the case that that site was mine, and the page that was broken was a dot-file that wouldn't show up in directory listings, and it only existed for about 30 seconds on the server, but during that time it was "a site which breaks", right?)

And I guess this should be marked fixed now, since the fix landed in m-c...
Closed: 13 years ago
Resolution: --- → FIXED
Is there a testcase so QA can verify this fix?
Whiteboard: [sg:critical] → [sg:critical][qa?]
Sure, see comment 0.  In an affected build, that testcase will *probably* print out a string filled with nonsense floating point numbers.  In a corrected build, at a glance I think it'd print "1234567bwhahaha".
Thanks Jeff, qa+
Whiteboard: [sg:critical][qa?] → [sg:critical][qa+]
Do we need to take this patch on the 1.9.2 branch, or is this code new since then?
[Triage comment]

This bug is being tracked for landing on ESR branch.  Please land patches on Thursday March 1, 2012 in order to be ready for go-to-build on Friday March 2, 2012.

See for more information.
Landed on ESR:

with the same commit message (and other bug number) as the one in comment 4, except that I appended "ESR a=dveditz" to the message.
This broke in bug 587257, which landed just under a year ago -- well after 1.9.2.
Jeff, trying to verify this on 10.0.3esr, running the Linux64 shell from:

I get the following output:
js> "1234567bwhahaha95.6e-3221.3796150552635895e-3061.0569372456451826e-3078.900719840310492e-3087.56546841585253e-3071.6021903463369686e-3069.457005280256007e-3087.78797919583446e-3089.346084319948922e-3078.90056706334849e-3081.223875502684094e-3071.390773018919187e-3080231.63e-32201.018557979663e-3121.6e-322"
Really?  I downloaded the from there and don't get that.  I also cloned esr10 and did a build against tip, and I don't see those results there either.  And shows the fix should be in the right branch (search for 720511 for the actual changeset corresponding to this bug).
My bad, I copied too much from comment 0. Just copying the code works for me.

Verified fixed in Firefox 10.0.3esr.
Verified fixed in Firefox 11.0 RC build 1.
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Verified fixed on OS X 10.7.3 using nightly trunk js-shell.
Group: core-security
Depends on: 720511
You need to log in before you can comment on or make changes to this bug.