720079 - array.join("") is GC-hazardous

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

/* 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("")
"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 1

[Jeff Walden [:Waldo]]

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

[Jeff Walden [:Waldo]]

Attachment: 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

[Alex Keybl [:akeybl]]

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

[Jeff Walden [:Waldo]]

Pushed to aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/1a931c313611

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

[u279076]

Is there a testcase so QA can verify this fix?

Comment 6

[Jeff Walden [:Waldo]]

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

[u279076]

Thanks Jeff, qa+

Comment 8

[Daniel Veditz [:dveditz]]

Do we need to take this patch on the 1.9.2 branch, or is this code new since then?

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage comment]

This bug is being tracked for landing on ESR branch.  Please land patches on http://hg.mozilla.org/releases/mozilla-esr10/by Thursday March 1, 2012 in order to be ready for go-to-build on Friday March 2, 2012.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more information.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Landed on ESR:

http://hg.mozilla.org/releases/mozilla-esr10/rev/7ab20b45eee8

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

[Jeff Walden [:Waldo]]

This broke in bug 587257, which landed just under a year ago -- well after 1.9.2.

Comment 12

[u279076]

Jeff, trying to verify this on 10.0.3esr, running the Linux64 shell from:
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/10.0.3esr-candidates/build1/

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"
"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

[Jeff Walden [:Waldo]]

Really?  I downloaded the jsshell-linux-x86_64.zip 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 http://hg.mozilla.org/releases/mozilla-esr10/graph/81725 shows the fix should be in the right branch (search for 720511 for the actual changeset corresponding to this bug).

Comment 14

[u279076]

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

[u279076]

Verified fixed in Firefox 11.0 RC build 1.

Comment 17

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

Verified fixed on OS X 10.7.3 using nightly trunk js-shell.