Last Comment Bug 720079 - array.join("") is GC-hazardous
: array.join("") is GC-hazardous
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- critical (vote)
: ---
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
: 735104 (view as bug list)
Depends on: 720511
  Show dependency treegraph
Reported: 2012-01-20 22:27 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-04-10 21:32 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch from bug 720511, applies cleanly to aurora/beta both (5.15 KB, patch)
2012-01-23 17:20 PST, Jeff Walden [:Waldo] (remove +bmo to email)
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-20 22:27:11 PST
/* 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("")
Comment 1 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-23 14:26:35 PST
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.
Comment 2 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-23 17:20:43 PST
Created attachment 590948 [details] [diff] [review]
Patch from bug 720511, applies cleanly to aurora/beta both

[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.
Comment 3 User image Alex Keybl [:akeybl] 2012-01-24 15:10:48 PST
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.
Comment 4 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-25 16:25:09 PST
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...
Comment 5 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-01 13:20:25 PST
Is there a testcase so QA can verify this fix?
Comment 6 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-01 14:20:28 PST
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".
Comment 7 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-01 14:22:18 PST
Thanks Jeff, qa+
Comment 8 User image Daniel Veditz [:dveditz] 2012-02-23 16:36:27 PST
Do we need to take this patch on the 1.9.2 branch, or is this code new since then?
Comment 9 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-02-23 16:46:28 PST
[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.
Comment 10 User image Gary Kwong [:gkw] [:nth10sd] 2012-02-24 15:56:24 PST
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.
Comment 11 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-24 16:20:14 PST
This broke in bug 587257, which landed just under a year ago -- well after 1.9.2.
Comment 12 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-05 15:35:52 PST
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"
Comment 13 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-05 16:11:33 PST
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).
Comment 14 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-05 16:22:35 PST
My bad, I copied too much from comment 0. Just copying the code works for me.

Verified fixed in Firefox 10.0.3esr.
Comment 15 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-10 15:03:36 PST
Verified fixed in Firefox 11.0 RC build 1.
Comment 16 User image Daniel Veditz [:dveditz] 2012-03-12 17:52:50 PDT
*** Bug 735104 has been marked as a duplicate of this bug. ***
Comment 17 User image Al Billings [:abillings] 2012-03-14 15:43:07 PDT
Verified fixed on OS X 10.7.3 using nightly trunk js-shell.

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