Last Comment Bug 743961 - Crash [@ JSStructuredCloneWriter::startWrite]
: Crash [@ JSStructuredCloneWriter::startWrite]
Status: RESOLVED FIXED
js-triage-needed
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla15
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2012-04-10 04:56 PDT by Christian Holler (:decoder)
Modified: 2012-05-07 01:32 PDT (History)
7 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Null-check context pushing callbacks. v1 (1.62 KB, patch)
2012-04-25 23:27 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-04-10 04:56:52 PDT
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] 2012-04-25 19:17:48 PDT
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) (busy with Stylo) 2012-04-25 23:12:18 PDT
(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) (busy with Stylo) 2012-04-25 23:27:07 PDT
Created attachment 618568 [details] [diff] [review]
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) (busy with Stylo) 2012-05-04 02:54:30 PDT
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) 2012-05-04 06:00:41 PDT
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) (busy with Stylo) 2012-05-05 09:35:47 PDT
Pushed this to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2650379278d2
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-05-05 09:39:29 PDT
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] 2012-05-05 20:34:54 PDT
https://hg.mozilla.org/mozilla-central/rev/2650379278d2
Comment 9 Alex Keybl [:akeybl] 2012-05-06 19:45:12 PDT
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) (busy with Stylo) 2012-05-07 01:32:28 PDT
Pushed to m-a:
http://hg.mozilla.org/releases/mozilla-aurora/rev/bf656bbd74bb

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