743961 - Crash [@ JSStructuredCloneWriter::startWrite]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test crashes on mozilla-central revision 434f50e70815 (no options required):


var n = (evalcx('lazy'));
var nbuf = serialize(n);


Valgrind trace:

==16319== Jump to the invalid address stated on the next line
==16319==    at 0x0: ???
==16319==    by 0x470636: JSStructuredCloneWriter::startWrite(JS::Value const&) (jsclone.cpp:568)
==16319==    by 0x470A4E: JSStructuredCloneWriter::write(JS::Value const&) (jsclone.cpp:605)
==16319==    by 0x46F369: js::WriteStructuredClone(JSContext*, JS::Value const&, unsigned long**, unsigned long*, JSStructuredCloneCallbacks const*, void*) (jsclone.cpp:67)
==16319==    by 0x443C52: JS_WriteStructuredClone (jsapi.cpp:5939)
==16319==    by 0x40F8FA: Serialize(JSContext*, unsigned int, JS::Value*) (js.cpp:3401)
==16319==    by 0x4F5A0E: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), js::CallArgs const&) (jscntxtinlines.h:314)
==16319==    by 0x4FBFFA: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:516)
==16319==    by 0x5082BE: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2685)
==16319==    by 0x69E877: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1079)
==16319==    by 0x69EA30: CheckStackAndEnterMethodJIT(JSContext*, js::StackFrame*, void*, bool) (MethodJIT.cpp:1111)
==16319==    by 0x69EAEF: js::mjit::JaegerShot(JSContext*, bool) (MethodJIT.cpp:1123)
==16319==  Address 0x0 is not stack'd, malloc'd or (recently) free'd



I'm not sure if this crash is shell-only or if the same situation can occur in the browser. Marking s-s until this is investigated and confirmed that it's either not in the browser or the invalid jump is always NULL.

Comment 1

[David Mandelin [:dmandelin]]

Bobby, this is crashing here:

class AutoEnterCompartmentAndPushPrincipal : public JSAutoEnterCompartment
{
  public:
    bool enter(JSContext *cx, JSObject *target) {
        // First, enter the compartment.
        if (!JSAutoEnterCompartment::enter(cx, target))
            return false;

        // We only need to push a principal if we changed compartments.
        if (state != STATE_OTHER_COMPARTMENT)
            return true;

        // Push.
        const JSSecurityCallbacks *cb = cx->runtime->securityCallbacks;
        return cb->pushContextPrincipal(cx, target->principals(cx));
               ^^^^^^^^^^^^^^^^^^^^^^^^
                 this is null == crash
    };

Is that just because we don't set the security callbacks in the shell? Is it always NULL and thus not s-s?

Comment 2

[Bobby Holley (:bholley)]

(In reply to David Mandelin from comment #1)
> Bobby, this is crashing here:
> 
> class AutoEnterCompartmentAndPushPrincipal : public JSAutoEnterCompartment
> {
>   public:
>     bool enter(JSContext *cx, JSObject *target) {
>         // First, enter the compartment.
>         if (!JSAutoEnterCompartment::enter(cx, target))
>             return false;
> 
>         // We only need to push a principal if we changed compartments.
>         if (state != STATE_OTHER_COMPARTMENT)
>             return true;
> 
>         // Push.
>         const JSSecurityCallbacks *cb = cx->runtime->securityCallbacks;
>         return cb->pushContextPrincipal(cx, target->principals(cx));
>                ^^^^^^^^^^^^^^^^^^^^^^^^
>                  this is null == crash
>     };
> 
> Is that just because we don't set the security callbacks in the shell? Is it
> always NULL and thus not s-s?

Ah yeah, should have null-checked the security callback. But yeah, I'm pretty sure we always run with security callbacks in the browser, and if we didn't this would just be a NULL deref (I don't see how we could get anything else in there, - do you?).

This code will go away once CPG lands, but might as well fix it to be safe. Patch coming up.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Null-check context pushing callbacks. v1

Attaching a patch. Blake, do you think this needs to be fixed on aurora?

Comment 4

[Bobby Holley (:bholley)]

Depending on whether blake thinks we need to fix this on aurora or not, we may not need this fix at all, since we can probably rip out these callbacks before the next release (since CPG landed).

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 618568 [details] [diff] [review]
Null-check context pushing callbacks. v1

If it's not too late to push this to Aurora, I'd say: go for it. It's a dead simple fix with little to no regression risk.

Also, I don't think this needs to be security sensitive, this bug is purely a null pointer dereference.

Comment 6

[Bobby Holley (:bholley)]

Pushed this to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2650379278d2

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 618568 [details] [diff] [review]
Null-check context pushing callbacks. v1

[Approval Request Comment]
Regression caused by (bug #): bug 739825
User impact if declined: Potential DoS by triggering (safe) crashes. Null pointer dereference.

Testing completed (on m-c, etc.): Just pushed to m-c, no testing yet. But it's a very safe patch - just a null check.
 
Risk to taking this patch (and alternatives if risky): Not much. See blake's analysis in comment 5.

String changes made by this patch: none

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2650379278d2

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 618568 [details] [diff] [review]
Null-check context pushing callbacks. v1

[Triage Comment]
Null check for a new regression caused in FF14. Approved for Aurora 14.

Comment 10

[Bobby Holley (:bholley)]

Pushed to m-a:
http://hg.mozilla.org/releases/mozilla-aurora/rev/bf656bbd74bb