925070 - Crash with Worker constructor in JSVAL_TO_STRING

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Nils]

HTML:
<script>
var w=new Worker('test.js');
w.constructor(0xbabababababa);
</script>

and an empty test.js will lead to the following crash (the crash address will change with the argument to the constructor).

=================================================================
==9522==ERROR: AddressSanitizer: SEGV on unknown address 0x575757575740 (pc 0x7f6cbb9d68f7 sp 0x7fff55ec70d0 bp 0x7fff55ec7300 T0)
AddressSanitizer can not provide additional info.
    #0 0x7f6cbb9d68f6 in isRope /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/String.h:304:0
    #1 0x7f6cbb9d68f6 in isLinear /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/String.h:315:0
    #2 0x7f6cbb9d68f6 in isFlat /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/String.h:337:0
    #3 0x7f6cbb9d68f6 in ensureFlat /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/String.h:1089:0
    #4 0x7f6cbb9d68f6 in JS_GetStringCharsZAndLength(JSContext*, JSString*, unsigned long*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/jsapi.cpp:5381:0
    #5 0x7f6cb76d55d2 in JSVAL_TO_STRING /builds/slave/m-cen-l64-asan-ntly-0000000000/build/dom/workers/../base/nsJSUtils.h:129:0
    #6 0x7f6cb76d55d2 in init /builds/slave/m-cen-l64-asan-ntly-0000000000/build/dom/workers/../base/nsJSUtils.h:141:0
    #7 0x7f6cb76d55d2 in (anonymous namespace)::Worker::ConstructInternal(JSContext*, JS::CallArgs, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/dom/workers/Worker.cpp:110:0
    #8 0x7f6cb76d5cbf in (anonymous namespace)::Worker::Construct(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/dom/workers/Worker.cpp:262:0
    #9 0x7f6cbb7c2b8a in native /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/jscntxtinlines.h:220:0
    #10 0x7f6cbb7c2b8a in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/Interpreter.cpp:463:0
    #11 0x7f6cbb7b3626 in Interpret(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/Interpreter.cpp:2465:0
    #12 0x7f6cbb7a4a21 in js::RunScript(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/Interpreter.cpp:420:0
    #13 0x7f6cbb7c4e0b in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/Interpreter.cpp:610:0
    #14 0x7f6cbb7c5166 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/Interpreter.cpp:646:0
    #15 0x7f6cbb9d15fd in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/jsapi.cpp:4918:0
    #16 0x7f6cb748b86f in nsJSUtils::EvaluateString(JSContext*, nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions&, JS::Value*, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/dom/base/nsJSUtils.cpp:280:0
    #17 0x7f6cb74795bf in nsJSContext::EvaluateString(nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, bool, JS::Value*, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/dom/base/nsJSEnvironment.cpp:995:0
    #18 0x7f6cb6c1860a in nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, nsString const&, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/base/src/nsScriptLoader.cpp:1002:0
    #19 0x7f6cb6c15f1c in nsScriptLoader::ProcessRequest(nsScriptLoadRequest*, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/base/src/nsScriptLoader.cpp:869:0
    #20 0x7f6cb6c152e2 in nsScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/base/src/nsScriptLoader.cpp:696:0
    #21 0x7f6cb6c0d319 in nsScriptElement::MaybeProcessScript() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/base/src/nsScriptElement.cpp:140:0
    #22 0x7f6cb79b8040 in operator-> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/obj-firefox/parser/html/../../dist/include/nsIScriptElement.h:220:0
    #23 0x7f6cb79b8040 in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/parser/html/nsHtml5TreeOpExecutor.cpp:789:0
    #24 0x7f6cb79b5bdb in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/parser/html/nsHtml5TreeOpExecutor.cpp:593:0
    #25 0x7f6cb792fbf8 in nsHtml5ExecutorFlusher::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/parser/html/nsHtml5StreamParser.cpp:131:0
    #26 0x7f6cb9e2a069 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/xpcom/threads/nsThread.cpp:622:0
    #27 0x7f6cb9d52241 in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/xpcom/glue/nsThreadUtils.cpp:238:0
    #28 0x7f6cb8a20f21 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/ipc/glue/MessagePump.cpp:85:0
    #29 0x7f6cb9f47fc3 in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/ipc/chromium/src/base/message_loop.cc:220:0
    #30 0x7f6cb9f47fc3 in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/ipc/chromium/src/base/message_loop.cc:213:0
    #31 0x7f6cb9f47fc3 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/ipc/chromium/src/base/message_loop.cc:187:0
    #32 0x7f6cb87f482c in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/widget/xpwidgets/nsBaseAppShell.cpp:161:0
    #33 0x7f6cb81e01ae in nsAppStartup::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/toolkit/components/startup/nsAppStartup.cpp:269:0
    #34 0x7f6cb567c2b0 in XREMain::XRE_mainRun() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/toolkit/xre/nsAppRunner.cpp:3868:0
    #35 0x7f6cb567d1ea in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/toolkit/xre/nsAppRunner.cpp:3936:0
    #36 0x7f6cb567e11b in XRE_main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/toolkit/xre/nsAppRunner.cpp:4138:0
    #37 0x459b8d in do_main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/browser/app/nsBrowserApp.cpp:275:0
    #38 0x459b8d in main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/browser/app/nsBrowserApp.cpp:635:0
    #39 0x7f6cc4d16ea4 in __libc_start_main /build/buildd/eglibc-2.17/csu/libc-start.c:260
    #40 0x45910c in _start ??:?
==9522==ABORTING

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like a regression from bug 643325.  The new code is:

    nsDependentJSString scriptURL;
    if (!scriptURL.init(aCx, aArgs[0])) {

but nsDependentJSString::init is:

139   bool init(JSContext* aContext, const JS::Value &v)
140   {
141       return init(aContext, JSVAL_TO_STRING(v));
142   }

which will, in this case, assert JSVAL_IS_STRING (which will fail for this case!) and then treat it as a a string.

Kyle, will bug 919885 fix this?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Looks like it will, since it will remove all this code.

Comment 3

[Andrew McCreight [:mccr8]]

This looks very similar to bug 923649.  Or at least, it is crashing in the same line in ConstructInternal: https://crash-stats.mozilla.com/report/index/cd01212f-d20e-4ab3-8f63-459a82131008

Comment 4

[Boris Zbarsky [:bzbarsky]]

That bug's testcase has:

  new Worker({toString:function(){}})

So yes, exactly the same issue.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'd prefer we fix this now rather than wait on bug 919885 though.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: changes.patch

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 815102 [details] [diff] [review]
changes.patch

r=me

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 815102 [details] [diff] [review]
changes.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.

Which older supported branches are affected by this flaw? None.

If not all supported branches, which bug introduced the flaw? Bug 643325.

How likely is this patch to cause regressions; how much testing does it need? Almost none, it restores old behavior.

Comment 9

[Andrew McCreight [:mccr8]]

For future reference, trunk-only bugs don't need sec-approval+.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8aea67a6a17c

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> For future reference, trunk-only bugs don't need sec-approval+.

Ah, ok. Thanks.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It should be fixed by the other bug, yes.  But lets land this anyways.

Comment 13

[Carsten Book [:Tomcat]]

fixed in https://hg.mozilla.org/mozilla-central/rev/8aea67a6a17c - do we also need a testcase for this ?

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

FIXED per comment 13.

Comment 15

[Andrew McCreight [:mccr8]]

type confusion sounds bad so I'm going to mark this sec-high.

Comment 16

[Matt Wobensmith [:mwobensmith][:matt:]]

I wasn't able to repro the original bug. If you get a chance Nils, it'd be great to verify that it no longer crashes for you. Thanks.