1126105 - Crash [@ JS::Value::toObjectOrNull] or [@ JSObject::isCallable]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

g = (function() p);
e1 = new Set;
a2 = [];
f2 = function() {};
m2 = new Map;
gczeal(0);
try {
    f = function() {
        this.i1 = a2;
        a2[4277] = 0;
        m2.set(g.h, e1);
        for each(w in [NaN, NaN, NaN, 0, objectEmulatingUndefined, objectEmulatingUndefined,
                       NaN, objectEmulatingUndefined(), 1, objectEmulatingUndefined(), NaN,
                       objectEmulatingUndefined, NaN, 1, NaN, objectEmulatingUndefined, NaN,
                       objectEmulatingUndefined(), NaN, NaN, 1, objectEmulatingUndefined(),
                       NaN, objectEmulatingUndefined, objectEmulatingUndefined,
                       objectEmulatingUndefined, objectEmulatingUndefined, 0, NaN, NaN, NaN,
                       objectEmulatingUndefined(), NaN, 0, objectEmulatingUndefined,
                       objectEmulatingUndefined, 0, NaN, objectEmulatingUndefined(), NaN,
                       NaN, 1, objectEmulatingUndefined(), NaN, NaN, 1,
                       objectEmulatingUndefined(), objectEmulatingUndefined(), NaN,
                       objectEmulatingUndefined, objectEmulatingUndefined, NaN, NaN,
                       objectEmulatingUndefined, objectEmulatingUndefined,
                       objectEmulatingUndefined, objectEmulatingUndefined, NaN,
                       objectEmulatingUndefined(), objectEmulatingUndefined(),
                       objectEmulatingUndefined(), objectEmulatingUndefined(), NaN, NaN,
                       objectEmulatingUndefined(), NaN, NaN, 1, NaN, objectEmulatingUndefined,
                       NaN, NaN, NaN, objectEmulatingUndefined(), 1, NaN, NaN, NaN, NaN,
                       objectEmulatingUndefined(), 1, objectEmulatingUndefined(), NaN,
                       objectEmulatingUndefined(), NaN, objectEmulatingUndefined,
                       objectEmulatingUndefined, objectEmulatingUndefined,
                       objectEmulatingUndefined, NaN, objectEmulatingUndefined(), 0,
                       objectEmulatingUndefined]) {
            for (v of i1) {
                try {
                    this.f2 = wrap(f2);
                } catch (e) {}
                f = function() {
                    try {} catch (e) {}
                };
            }
        }
        e1.valueOf = function() {
            ccfxxe = 0;
            return function() {
                f2(ccfxxe);
            };
        }();
        b2 = m2.get(this.s2);
        b2 + 2;
    };
    f();
} catch (e) {}

crashes js debug shell on m-c changeset 95c76c3b0172 with --fuzzing-safe --no-threads --ion-eager at JS::Value::toObjectOrNull with JSObject::isCallable on the stack.

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Comment 1

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

Attachment: stack

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ed2fb19942d0
parent:      204702:bc7deafdac4b
user:        Eric Faust
date:        Wed Sep 10 15:52:36 2014 -0700
summary:     Bug 966518 - Part 0: Make proxy callability into a trap, rather than a class check. (r=bholley, r=djvj, r=peterv)

This iteration took 293.602 seconds to run.

Comment 3

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

Eric, is bug 966518 a likely regressor?

Comment 4

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

function f() {}
x = []
x[999999] = 0;
for (v of x) {
    f = wrap(f)
}
f()

crashes js debug shell on m-c changeset 08e41ea36f6d with --fuzzing-safe --no-threads --ion-eager at JS::Value::toObjectOrNull with JSObject::isCallable on the stack.

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Comment 5

[Eric Faust [:efaust]]

Yeah, it's a likely regressor, but for a stupid reason.

The second reduced test makes it painfully obvious that we need a recursion check inside DirectProxyHandler::isCallable. That's more than a one liner because isCallable() doesn't currently take a cx.

Comment 6

[Eric Faust [:efaust]]

"More than a one liner" he said. "I'll just take care of this this afternoon" he said.

On try, finally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=01ee9caab300

Comment 7

[Eric Faust [:efaust]]

Attachment: Fix

OK, looks like I got the issues on Android sorted out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae2b7fbb39df

Jeff, can you pay special attention to the removal of the assert in CallArgs::isConstructing()? Probably we could add it back, and make it take a cx, since all the callers are likely inside JSNatives. Is that worth it?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8557447 [details] [diff] [review]
Fix

I think the codegen changes to getJSToNativeConversionInfo would be a lot more readable with fill().

>@@ -13633,17 +13657,23 @@ class CGCallback(CGClass):
>+            MOZ_ASSERT(js::VerifyIsCallableInAssert(jsapi.cx(), aCallback, true));

Do we not need cx in the compartment of aCallback?  Seems like we should enter that compartment....  We should add a test here that would exercise the proxy case taking this codepath, since I bet that would hit compartment asserts.

>+++ b/ipc/ril/Ril.cpp

Don't you need to report the pending exception, like the code above you?

>+++ b/js/xpconnect/wrappers/WrapperFactory.cpp

>-                               CompartmentPrivate::Get(target)->wantXrays;
>+                            CompartmentPrivate::Get(target)->wantXrays;

Why the indentation change?

r=me with the above fixed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Though I'd like to make sure I understand the reason this can fail.

Is this about having a long direct proxy chain, such that doing DirectProxyHandler::isCallable runs out of stack and throws?

I'd think for non-scripted proxies we can tie callability to the JSClass.  For scripted ones, could we just store it in a reserved slot or equivalent and not have to deal with IsCallable maybe failing?  Certainly per spec it can't fail...

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8557447 [details] [diff] [review]
Fix

Review of attachment 8557447 [details] [diff] [review]:
-----------------------------------------------------------------

So if really wrap() is the only offender here, I guess this isn't useful.  But might as well post it since it's done, in case we change our minds.

::: dom/bindings/Codegen.py
@@ +4990,5 @@
> +                # We'd love to just name the bool according to declName, but we
> +                # change that on the fly when we instantiate, so it's not stable
> +                # as a name for something else. Instead, stash the calculation
> +                # in a block.
> +                "{\n" +

Egad please for the love of all that is holy convert this to a dedent("""...""") this is unreadable why are you doing this to me aaaaaaughghghghghgh *catatonia*.

;-)

@@ +13666,5 @@
> +            #ifdef DEBUG
> +            AutoJSAPI jsapi;
> +            jsapi.Init();
> +            MOZ_ASSERT(js::VerifyIsCallableInAssert(jsapi.cx(), aCallback, true));
> +            #endif

Precedent from the HasPropertyOnPrototype cases I wrote in this file recently suggest these should be indented four spaces from the "b" in |body| to be readable.  Although with inexact assertions again, we needn't indent at all, I think.

@@ +14141,5 @@
> +            """
> +            bool isCallable;
> +            if (!JS::IsCallable(cx, callable, &isCallable)) {
> +              aRv.Throw(NS_ERROR_UNEXPECTED);
> +              return${errorReturn};

Missing space between |return| and |$|.

@@ +14143,5 @@
> +            if (!JS::IsCallable(cx, callable, &isCallable)) {
> +              aRv.Throw(NS_ERROR_UNEXPECTED);
> +              return${errorReturn};
> +            }
> +            """,

Should be

    return fill("""
        bool isCallable;
        ...
        """,
        errorReturn=self.getDefaultRetval())

as far as formatting goes.

@@ +14197,5 @@
>          if not self.singleOperation:
>              return 'JS::Rooted<JS::Value> callable(cx);\n' + getCallableFromProp
>          return fill(
>              """
> +            bool isCallable;

Or, hmm.  I guess this file is inconsistent about fill("""""") style.  I would prefer the """-on-fill-line style, myself.  bz has no opinion on the matter.  Should probably mass-fix the file in a separate commit (and bug, if a bug is warranted for a whitespace-only fix).

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +800,3 @@
>      if (!GetProperty(cx, jsobj, method, &fv) ||
> +        !::JS_TypeOfValue(cx, fv, &type) ||
> +        type != JSTYPE_FUNCTION) {

Not equivalent.  In the case where the type isn't function, this will silently halt execution.  You get to be the person to introduce actual JSAPI-compliant error handling here.

JSType type;
if (!GetProperty(cx, jsobj, method, &fv) ||
    !::JS_TypeOfValue(cx, fv, &type))
{
  return false;
}
if (type != JSTYPE_FUNCTION) {
  ...report some sort of error here
} else {
  fv.setObject(*jsobj);
}

::: js/public/CallArgs.h
@@ -175,5 @@
>  
>      bool isConstructing() const {
> -#ifdef JS_DEBUG
> -        if (this->usedRval_)
> -            CheckIsValidConstructible(calleev());

Yeah, it'd be nice to keep this, but the existence of callability/constructibility methods in jsapi.h makes this Hard-ish.  Let's punt to a followup, yeah.

::: js/src/jit/Lowering.cpp
@@ +1032,5 @@
>  
>      LTypeOfV *lir = new(alloc()) LTypeOfV(tempToUnbox());
>      useBox(lir, LTypeOfV::Input, opd);
>      define(lir, ins);
> +    assignSafepoint(lir, ins);

I'm going to assume this is just the thing you do after you've defined a fallible instruction.  Looks it judging by all the other uses in Lowering.cpp, but I don't know JIT enough to say that confidently myself.

::: js/src/jsapi.cpp
@@ +3459,5 @@
> +    return true;
> +}
> +
> +JS_PUBLIC_API(bool)
> +IsCallable(JSContext *cx, HandleValue v, bool *out)

This isn't necessary if the js::IsCallable is moved into public API as an inline, as I noted elsewhere.

@@ +3471,5 @@
> +}
> +
> +
> +JS_PUBLIC_API(bool)
> +IsConstructor(JSContext *cx, HandleObject obj, bool *out)

I'd also move this into jsapi.h as an inline, but I imagine this is a much less common operation, so don't bother if it requires tricky to handle all the casts and random internal guts that must be checked to implement the stuff below.

::: js/src/jscompartment.cpp
@@ +430,3 @@
>          {
> +            bool callable;
> +            if (!IsCallable(cx, obj, &callable))

You lost a callability check on |existing| here.

::: js/src/jsfriendapi.cpp
@@ +1254,5 @@
> +
> +#ifdef DEBUG
> +
> +JS_FRIEND_API(bool)
> +js::VerifyIsCallableInAssert(JSContext *cx, HandleObject obj, bool expected)

These all shouldn't be needed once we have the CheckCallable/CheckConstructible methods I suggest elsewhere, that settle for being unable to give a precise answer in rare cases.

::: js/src/jsobj.cpp
@@ +2876,2 @@
>  {
> +    *hook = nullptr;

All the comments from ObjectConstructHook apply here, mutatis mutandis.

@@ +2898,2 @@
>  {
> +    *hook = nullptr;

I would prefer not to have pre-init like this.  Callers have to check the return value.  If they're not, they're doing it wrong.  Doing this lets sloppy code slide longer.

@@ +2905,3 @@
>      }
> +
> +    if (obj->is<js::ProxyObject>()) {

Better to invert and have an early exit:

    if (!obj->is<js::ProxyObject>()) {
        *hook = nullptr;
        return true;
    }

    bool construct;
    ...
    *hook = construct ? js::proxy_Construct : nullptr;
    return true;

::: js/src/jsobj.h
@@ +709,3 @@
>  // ES6 rev 24 (2014 April 27) 7.2.5 IsConstructor
>  inline bool
> +IsConstructor(JSContext *cx, HandleValue v, bool *out)

Move this into jsapi.h in namespace JS?

::: js/src/json.cpp
@@ +494,5 @@
> +        MOZ_ASSERT(!cx->isExceptionPending());
> +        MOZ_ASSERT(IsFilteredValue(cx, val, &filtered) ||
> +                   cx->isExceptionPending());
> +        MOZ_ASSERT_IF(!cx->isExceptionPending(), !filtered);
> +        cx->clearPendingException();

This...is kind of monstrous.  I mean, *in theory* okay.  But I am extraordinarily leery about having a method that, *in release builds*, silently clears a pending exception if this happened to be called in that already-in-error state.

I would rather see a method that checks filterability but doesn't try to *exactly* answer the question:

enum Filterability { Filtered, Unfiltered, Nonobvious };
void CheckFilterable(const Value &v)
{
  if (v.isUndefined() || v.isSymbol())
    return Filtered;

  if (!v.isObject())
    return Unfiltered;

  if (v.toObject().is<ProxyObject>())
    return Nonobvious;

  // ...do an exact check...
}

Then MOZ_ASSERT(CheckFilterable(v) != Unfiltered); without any of this *actual* effect on non-debug execution.

Perhaps the last few lines here should be encapsulated into JSAPI.

enum Callability { Callable, Noncallable, Nonobvious };

inline void
CheckCallability(const Value &v)
{
  if (!v.isObject())
    return Noncallable;
  if (v.toObject().is<ProxyObject>())
    return Nonobvious;
  ...exact check maybe returning Callable...
}

That would let all the existing assertions, still assert sanity in most cases.

::: js/src/jsproxy.h
@@ +406,5 @@
>                                     unsigned indent) const MOZ_OVERRIDE;
>      virtual bool regexp_toShared(JSContext *cx, HandleObject proxy,
>                                   RegExpGuard *g) const MOZ_OVERRIDE;
>      virtual bool boxedValue_unbox(JSContext *cx, HandleObject proxy, MutableHandleValue vp) const MOZ_OVERRIDE;
> +    virtual bool isCallable(JSContext *cx, HandleObject proxy, bool *callable) const MOZ_OVERRIDE;

Strange that isConstructor isn't also overloaded, maybe.

::: js/src/proxy/BaseProxyHandler.cpp
@@ +397,3 @@
>  {
> +    *out = false;
> +    return true;

This isn't really happymaking, but I guess we can fix this when we convert traps from a vtable-based system to an explicit function pointer table system.  Same for isConstructor.

::: js/src/proxy/CrossCompartmentWrapper.cpp
@@ +424,5 @@
>  
> +bool
> +CrossCompartmentWrapper::isCallable(JSContext *cx, HandleObject wrapper, bool *callable) const
> +{
> +    JS_CHECK_RECURSION(cx, fprintf(stderr, "what the fuck?!\n"); return false);

I'm gonna have to ask you to remove that before landing.

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +1213,5 @@
>          return false;
>      Rooted<ProxyObject*> proxy(cx, &proxy_->as<ProxyObject>());
>      proxy->setExtra(ScriptedDirectProxyHandler::HANDLER_EXTRA, ObjectValue(*handler));
>  
>      // Assign [[Call]] and [[Construct]]

Maybe worth a comment that because these aren't script-trappable, either ordering of calls is fine here, because an error in one is indistinguishable ordering-wise from an error in the other.

::: js/src/vm/Debugger.cpp
@@ +2606,5 @@
> +    if (!args[0].isNull()) {
> +        bool callable;
> +        if (!IsCallable(cx, args[0], &callable))
> +            return false;
> +        if (!args[0].isObject() || !callable) {

You can remove the first condition here, as callable implies object.

@@ +5901,5 @@
> +    bool valid;
> +    MOZ_ASSERT(IsValidHook(cx, v, &valid) || cx->isExceptionPending());
> +    MOZ_ASSERT_IF(!cx->isExceptionPending(), valid);
> +    cx->clearPendingException();
> +    return true;

This should also go into the CheckCallable() != Noncallable assertion form, and cx argument dropt and all.

::: js/src/vm/Interpreter.cpp
@@ +582,4 @@
>  
>          if (fun->isNativeConstructor()) {
>              bool ok = CallJSNativeConstructor(cx, fun->native(), args);
>              return ok;

Mind making this just |return CallJSNativeConstructor(...);| while you're in the area?

@@ +2370,5 @@
>  CASE(JSOP_TYPEOFEXPR)
>  CASE(JSOP_TYPEOF)
>  {
> +    RootedValue &val = rootValue0;
> +    val = REGS.sp[-1];

HandleValue val = REGS.stackHandleAt(-1);

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ +160,5 @@
>      XPCCallContext ccx(JS_CALLER, cx, obj);
>      XPCWrappedNative* wrapper = ccx.GetWrapper();
>      THROW_AND_RETURN_IF_BAD_WRAPPER(cx, wrapper);
>  
> +    MOZ_ASSERT(AssertHasType(cx, args.calleev(), JSTYPE_FUNCTION));

This assertion is basically ridiculous, please remove.  :-)

@@ +1165,5 @@
>  bool
>  XPC_WN_CallMethod(JSContext *cx, unsigned argc, jsval *vp)
>  {
>      JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(AssertHasType(cx, args.calleev(), JSTYPE_FUNCTION));

And this.

@@ +1191,5 @@
>  bool
>  XPC_WN_GetterSetter(JSContext *cx, unsigned argc, jsval *vp)
>  {
>      JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(AssertHasType(cx, args.calleev(),JSTYPE_FUNCTION));

And this.

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +406,5 @@
>      XrayType xrayType = GetXrayType(obj);
>  
>      const Wrapper *wrapper;
>  
> +    do {

I agree with bz's apparent silent assent as to the correctness of the (highly security-sensitive) changes in this file.

@@ +478,5 @@
>          //
>          // Xrays are a bidirectional protection, since it affords clarity to the
>          // caller and privacy to the callee.
>          bool sameOriginXrays = CompartmentPrivate::Get(origin)->wantXrays ||
> +                            CompartmentPrivate::Get(target)->wantXrays;

Undo the change here.

@@ +496,5 @@
>          // If we want to apply add-on interposition in the target compartment,
>          // then we try to "upgrade" the wrapper to an interposing one.
>          if (CompartmentPrivate::Get(target)->scope->HasInterposition())
>              wrapper = SelectAddonWrapper(cx, obj, wrapper);
> +    } while(false);

} while (false); (extra space)

Comment 11

[Eric Faust [:efaust]]

Attachment: Much easier fix

Boris makes a compelling point. Since I don't believe this can happen in the wild, let's instead fix it by disallowing the fuzzers to interact with these testing functions. wrapWithPrototype tests a delicate functionality, so allow it as fuzzing unsafe, but wrap() should go.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 8558829 [details] [diff] [review]
Much easier fix

Review of attachment 8558829 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/shell/js.cpp
@@ +4669,5 @@
> +    JS_FN_HELP("wrapWithProto", WrapWithProto, 2, 0,
> +"wrapWithProto(obj)",
> +"  Wrap an object into a noop wrapper with prototype semantics.\n"
> +"  Note: This is not fuzzing safe because it can be used to construct\n"
> +"        deep nested wrapper chains that cannot exist in the wild."),

deep-nested

Comment 13

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0afaf38c7203

Let's start with this. If we come up with some need for the other patch, we can bring it back.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8557447 [details] [diff] [review]
Fix

Review of attachment 8557447 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review as we don't need this patch (for now, at least).

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0afaf38c7203