The default bug view has changed. See this FAQ.

array.join("") is GC-hazardous

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(firefox10 wontfix, firefox11 verified, firefox-esr1011+ verified, status1.9.2 unaffected)

Details

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

Attachments

(1 attachment)

/* 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"
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.
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.
Attachment #590948 - Flags: approval-mozilla-beta?
Attachment #590948 - Flags: approval-mozilla-aurora?

Comment 3

5 years ago
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:

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...
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox11: --- → fixed
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?
status-firefox-esr10: --- → affected
status-firefox10: --- → wontfix
tracking-firefox-esr10: --- → 11+
[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.
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.
status-firefox-esr10: affected → fixed
This broke in bug 587257, which landed just under a year ago -- well after 1.9.2.
status1.9.2: --- → unaffected
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"
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).
My bad, I copied too much from comment 0. Just copying the code works for me.

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