1609432 - Reconsider unsoundness when eagerly evaluating higher order native functions

Status: RESOLVED FIXED

(DevTools :: Console, task, P3)

Description

[Brian Hackett [Laid off!]]

If a higher order self hosted native function is passed a native which has side effects, that native can be called without invoking the debugger's onNativeCall hook. This can allow side effects to occur when eagerly evaluating something like "[1,2,3].map(Array.prototype.push)".

This unsoundness could be fixed by keeping track of which natives are higher order and vulnerable to this problem, and force-terminating the eager evaluation if any of them are called with an effectful native. This is pretty obscure though and might not be worth worrying about or incurring the complexity of a proper fix.

Comment 2

[Oriol Brufau [:Oriol]]

[1,2,3].map(Array.prototype.push) seems to just throw TypeError: "can't convert undefined to object".

I think you meant something like [1,2,3].map(Array.prototype.push, Array.prototype), then Array.prototype.length becomes 9.

Comment 3

[cpplearner]

I think this is what causes a bunch of prompt boxes to be opened when I type [1, 2, 3].map(prompt) (where prompt comes from autocompletion), which is very annoying.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

arai, since we're looking at this area today, would you know how we could implement what is described in Comment 0?
I don't know how we can know if a native is higher order in the first place, and I'm not sure if the plan laid out is feasible only in JS.

Note that Bug 1625069 describe what looks like a more robust way of doing eager evaluation, but this is definitely not something that we (DevTools team) could do as we don't have much C++ knowledge at the moment.

Comment 5

[Tooru Fujisawa [:arai]]

We could expose the call's arguments to onNativeCall here.

https://searchfox.org/mozilla-central/rev/fa71140041c5401b80a11f099cc0cd0653295e2c/js/src/debugger/Debugger.cpp#2285-2287,2315

bool Debugger::fireNativeCall(JSContext* cx, const CallArgs& args,
                              CallReason reason, ResumeMode& resumeMode,
                              MutableHandleValue vp) {
...
  bool ok = js::Call(cx, fval, object, calleeval, reasonval, &rv);

So, as long as we could maintain the list of either "safe" or "danger" functions, it could work, but I'm not sure if that's sustainable.

Also, there's other possibility.
onNativeCall inside self-hosted is skipped here.

https://searchfox.org/mozilla-central/rev/fa71140041c5401b80a11f099cc0cd0653295e2c/js/src/debugger/Debugger.cpp#965-976

// The onNativeCall hook is fired when self hosted functions are called,
// and any other self hosted function or C++ native that is directly called
// by the self hosted function is considered to be part of the same
// native call.
//
// We check this only after checking that debuggerList has items in order
// to avoid unnecessary calls to cx->currentScript(), which can be expensive
// when the top frame is in jitcode.
JSScript* script = cx->currentScript();
if (script && script->selfHosted()) {
  return NativeResumeMode::Continue;
}

if it's really necessary, I think we could either:

• (a) add a flag to debugger or something not to skip call inside self-hosted functions
• (b) call some different hook (e.g. onNativeCallInSelfHosted) for such case
• (c) (a) + (b)

Comment 6

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #5)

We could expose the call's arguments to onNativeCall here.

something like the following:

diff --git a/js/src/debugger/Debugger.cpp b/js/src/debugger/Debugger.cpp
index c5de834a41977..92e9e90efb95d 100644
--- a/js/src/debugger/Debugger.cpp
+++ b/js/src/debugger/Debugger.cpp
@@ -2311,8 +2311,28 @@ bool Debugger::fireNativeCall(JSContext* cx, const CallArgs& args,
 
   RootedValue reasonval(cx, StringValue(reasonAtom));
 
+  Rooted<ArrayObject*> argArray(cx, NewDenseEmptyArray(cx));
+  if (!argArray) {
+    return false;
+  }
+  for (unsigned i = 0; i < args.length(); i++) {
+    RootedValue arg(cx, args[i]);
+    if (!wrapDebuggeeValue(cx, &arg)) {
+      return false;
+    }
+    if (!NewbornArrayPush(cx, argArray, arg)) {
+      return false;
+    }
+  }
+
+  FixedInvokeArgs<3> invokeArgs(cx);
+  invokeArgs[0].set(calleeval);
+  invokeArgs[1].set(reasonval);
+  invokeArgs[2].set(ObjectValue(*argArray));
+
+  RootedValue objval(cx, ObjectValue(*object));
   RootedValue rv(cx);
-  bool ok = js::Call(cx, fval, object, calleeval, reasonval, &rv);
+  bool ok = js::Call(cx, fval, objval, invokeArgs, &rv);
 
   return processHandlerResult(cx, ok, rv, NullFramePtr(), nullptr, resumeMode,
                               vp);

I'll look into onNativeCall inside self-hosted case.

Comment 7

[Tooru Fujisawa [:arai]]

The issue with calling onNativeCall or variant inside self-hosted code is that,
the callee can be self-hosted JS's internal function, that's not exposed elsewhere.

https://searchfox.org/mozilla-central/rev/fa71140041c5401b80a11f099cc0cd0653295e2c/js/src/builtin/Array.js#156,158,161,166,173

function ArrayMap(callbackfn/*, thisArg*/) {
...
    var O = ToObject(this);
...
    var len = ToLength(O.length);
...
    if (!IsCallable(callbackfn))
...
    var A = ArraySpeciesCreate(O, len);

So, the nativeHasNoSideEffects check might not work, or needs special handling.

Comment 8

[Tooru Fujisawa [:arai]]

Possible workaround for the onNativeCall inside self-hosted JS is to hook into callContentFunction syntax, and call onNativeCall or variant only for the function call with it.
This needs slight change on the bytecode and how the call there works, but at least can avoid exposing self-hosted JS's internal functions.

Comment 9

[Tooru Fujisawa [:arai]]

Calling onNativeCall for callContentFunction fixes the comment #2 and bug 1754141,
but not bug 1732543 and bug 1777241.

Bug 1777241 testcase uses std_Function_apply, that needs yet another special handling.

https://searchfox.org/mozilla-central/rev/c11f54459452dd2f9ab2f9bec4ae03127897d256/js/src/builtin/Reflect.js#31,43

function Reflect_apply(target, thisArgument, argumentsList) {
...
    return callFunction(std_Function_apply, target, thisArgument, argumentsList);

Bug 1732543 testcase uses Function.prototype.call that also needs yet another special handling.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Bug 1609432 - Part 1: Add more functions to eager evaluation allowlist. r?nchevobbe!

RegExp.prototype methods called inside String.prototype.{match,search,replace}
should be allowed.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Bug 1609432 - Part 2: Always use callContentFunction and constructContentFunction for possibly user-provided functions. r?jandem!

Use callContentFunction even if this value is undefined.
Use constructContentFunction even if newTarget value is constructot itself.

Depends on D152343

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Bug 1609432 - Part 3: Add JSOp::CallContent and JSOp::NewContent. r?jandem!

These instructions are for callContentFunction and constructContentFunction
in self-hosted JS, to notify possible native call to debugger in the later
patches.

Depends on D152344

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Bug 1609432 - Part 4: Add CallReason::CallContent. r?jandem!

Depends on D152345

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Bug 1609432 - Part 5: Call onNativeCall for callContentFunction and constructContentFunction in self-hosted JS. r?jandem!

Depends on D152346

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Bug 1609432 - Part 6: Add eager evaluation testcase. r?nchevobbe!

Depends on D152347

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a7886cefc68
Part 1: Add more functions to eager evaluation allowlist. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/2825d06d411b
Part 2: Always use callContentFunction and constructContentFunction for possibly user-provided functions. r=jandem
https://hg.mozilla.org/integration/autoland/rev/58d74b50b7e7
Part 3: Add JSOp::CallContent and JSOp::NewContent. r=jandem
https://hg.mozilla.org/integration/autoland/rev/ba24faeb44f5
Part 4: Add CallReason::CallContent. r=jandem
https://hg.mozilla.org/integration/autoland/rev/54bafa2d55b9
Part 5: Call onNativeCall for callContentFunction and constructContentFunction in self-hosted JS. r=jandem
https://hg.mozilla.org/integration/autoland/rev/df0bf2b761a7
Part 6: Add eager evaluation testcase. r=nchevobbe

Comment 18

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/0a7886cefc68
https://hg.mozilla.org/mozilla-central/rev/2825d06d411b
https://hg.mozilla.org/mozilla-central/rev/58d74b50b7e7
https://hg.mozilla.org/mozilla-central/rev/ba24faeb44f5
https://hg.mozilla.org/mozilla-central/rev/54bafa2d55b9
https://hg.mozilla.org/mozilla-central/rev/df0bf2b761a7