Emscripten: Remove NO_DYNAMIC_EXECUTION option

[nazar-pc]

Once #5751 is merged NO_DYNAMIC_EXECUTION would either be no-op or very close to that.
Should be possible to remove it as redundant entirely.

[nazar-pc]

There are a few things on the way here. Without them NO_DYNAMIC_EXECUTION can be dropped with minor tests changes where tests are aware of this option.

The first is makeEval() that is still there and uses eval()

One usage is for loading asm.js, which:
1) Can be left as is (I'm not really sure how long it will/should be supported, since there is WebAssembly everywhere nowadays) since it doesn't affect WebAssembly builds
2) Can actually be fixed natively without eval(), but I don't want to spend time on it honestly

Second usage is some unfamiliar code in library.js:
https://github.com/kripken/emscripten/blob/cfe60eef389f70f7ddbd309d286d1dcdd05e2585/src/library.js#L3940-L3963
I can't imagine how to make this brutal code CSP-friendly, since it is literally supposed to call eval() directly. Can someone name a use case where it is not possible to avoid this?

Second is Embind, which also uses eval(), but in some cases have fallbacks that do not use eval()

Here there is a strange createNamedFunction. It looks like this function can work with NO_DYNAMIC_EXECUTION:
https://github.com/kripken/emscripten/blob/cfe60eef389f70f7ddbd309d286d1dcdd05e2585/src/embind/embind.js#L185-L204

However, then I found this:
https://github.com/kripken/emscripten/blob/cfe60eef389f70f7ddbd309d286d1dcdd05e2585/src/embind/embind.js#L783-L814

Would be nice if someone can explain what is this doing.

[kripken]

I think we can focus on wasm, yeah. That's what matters going forward.

emscripten_run_script is basically a direct path to eval, yeah. Sometimes people want this, if they generate the JS at runtime. It's rare, and we can warn about this in the docs. I don't think we need to do any work on it.

I'm not familiar with the embind code. Perhaps @AlexPerrot knows?

[buu700]

re: emscripten_run_script, is that actually used anywhere at run time? From a quick grep yesterday, it looked like it was only directly referenced by the tests and/or possibly at build time.

[kripken]

Well, people might be using the API in their own projects. Hard to say how popular it is, we could ask on the mailing list perhaps. But in any case, for people not using it, it doesn't do any harm, so it shouldn't be a problem for the goal in this issue?

[buu700]

Cool, yeah, that's what I was thinking. If it's not depended on by the emscripten runtime then it seems like it wouldn't be a problem for NO_DYNAMIC_EXECUTION.

[nazar-pc]

From option description NO_DYNAMIC_EXECUTION disables some functionality and causes runtime errors, which is quite easy to identify. After removing option entirely there will be no descriptive exception or warning and instead we'll see native Content Security Policy violation instead whenever eval() or new Function is used, which might be harder to debug.

The only practical difference would be when build is tested in non-CSP environment (like Node.js for instance on CI server) and deployed in CPS environment that forbids eval(). Then there will be an error in production, but tests would be green.

Effectively most of the builds should already be free from eval() regardless of NO_DYNAMIC_EXECUTION value anyway.

I'll start with removing conditionals where possible first, but not sure it is feasible to remove this option entirely.

[nazar-pc]

I think I've removed it in https://github.com/nazar-pc/emscripten/tree/NO_DYNAMIC_EXECUTION-elimination-1, will submit PR once #5934 is merged, currently waiting on CI in my fork.

Here is how mentioned branch from my fork behaves:

• NO_DYNAMIC_EXECUTION is non-existent except in changelog 👌
• eval() will be present in asm.js build, but will not be executed if you load asm.js file manually
  
  
  
  • whoever did this already will not notice any difference
  
  
  • whoever didn't will see an error just like before, but native CSP violation instead of Emscripten's exception (the only exception is -s NO_DYNAMIC_EXECUTION=2, but I think we can live without it)
  
  
  • it is possible to remove eval() entirely here, but I'll not waste my time on this, good luck if you think it is worth your effort (I can suggest an approach if someone is interested)
  
  
• emscripten_run_script, emscripten_run_script_int and emscripten_run_script_string will naturally emit eval() and there is literally no way around it, but at this point I hope you know what you're doing and NO_DYNAMIC_EXECUTION wouldn't help you much here
• no eval() or new Function otherwise

[kripken]

Overall that sounds good, although I'm not sure yet what eval() will be present in asm.js build means.

[nazar-pc]

Here is relevant snippet of code with that eval():

#if BINARYEN_METHOD != 'native-wasm'
  function doJustAsm(global, env, providedBuffer) {
    // if no Module.asm, or it's the method handler helper (see below), then apply
    // the asmjs
    if (typeof Module['asm'] !== 'function' || Module['asm'] === methodHandler) {
      if (!Module['asmPreload']) {
        // you can load the .asm.js file before this, to avoid this sync xhr and eval
        eval(Module['read'](asmjsCodeFile));
      } else {
        Module['asm'] = Module['asmPreload'];
      }
    }
    if (typeof Module['asm'] !== 'function') {
      Module['printErr']('asm evalling did not set the module properly');
      return false;
    }
    return Module['asm'](global, env, providedBuffer);
  }
#endif // BINARYEN_METHOD != 'native-wasm'

[kripken]

Oh, right, for loading asm.js in wasm mode, we do need it. That's a pretty rare case, though (normal asm.js doesn't use it, nor wasm in wasm mode), so it's kind of like emscripten_run_script, we can ignore it here, it will only have an effect on people specifically using those rare features.

[saschanaz]

Regarding to createNamedFunction, that code effectively does nothing when NO_DYNAMIC_EXECUTION, which means it really creates non-named anonymous function.

[nazar-pc]

Yes, it does nothing in incoming. In my fork it is actually behaves as named function: https://github.com/kripken/emscripten/compare/incoming...nazar-pc:NO_DYNAMIC_EXECUTION-elimination-1#diff-1e25583510869c425881c98eac3b295bR179

[saschanaz]

I think the only reason to use createNamedFunction is for stack tracing. Object.defineProperty does change the name property but AFAIK the change does not appear on stack trace.

[nazar-pc]

I guessed it was for debugging, but wasn't aware about stack trace.

With stack trace the most useful thing I've got is following:

function make_my_func (func) {
    Object.defineProperty(func, 'name', {value: 'my_func'});
    func.displayName = 'my_func';
    func.toString = function () {return 'function my_func () {}'};
    var obj = {};
    obj['my_func'] = func;
    return obj['my_func'].bind(obj);
}
console.log(make_my_func(function () {console.log(new Error);})())

But it doesn't work in Firefox at all.

Looks like createNamedFunction is not longer useful with proposed change.

[saschanaz]

So it seems Object.defineProperty works for stack traces on Node.js/Chrome but not on others.

There are some other tricks but all these are buggy on Firefox. 🤔

var dynamicName = "wow";
var obj = {};

// appears as `function obj[dynamicName]()` on stack trace and the name property is empty
obj[dynamicName] = function() { throw new Error("wow") }

// ES2015+ only. Appears as `function obj()` on stack trace and the name property is `wow`
obj = { [dynamicName]() { throw new Error("wow") } }

[nazar-pc]

No, Object.defineProperty is only used for Emscripten, it is ignored by both Firefox and Chromium for anything.
.toString works in Chromium, .displayName works in Firefox, but only for console.log(func), not for stack trace.

The only things that works for stack trace in Chromium is obj[dynamicName], but it doesn't work in Firefox.

I'm not sure what to do here. It feels like too much pride for eval() just for this feature, but debugging will become a bit more difficult in some cases.

[saschanaz]

No, Object.defineProperty is only used for Emscripten, it is ignored by both Firefox and Chromium for anything.

On Node 8:

> function namedWrapper(name, func) { var newFunc = function() { return func.apply(this, arguments) }; Object.defineProperty(newFunc, "name", { value: name }); return newFunc; }
undefined
> namedWrapper("wow", function() { return 3 })
[Function: wow]
> namedWrapper("wow", function() { throw new Error() })()
Error
    at repl:1:40
    at wow (repl:1:76)

I hope we can do some mixed method to make it work (nearly) everywhere but it seems hard on Firefox. And we are yet to mention ChackraCore and JSC.

PS: Opened a bug for Firefox (although it's for ES2015) https://bugzilla.mozilla.org/show_bug.cgi?id=1427228

[ffflorian]

Hi everyone,
thanks for your great work on emscripten!

I hope it's okay if I throw in a question into this issue since I am quite sure it's related.

Since libsodium.js included WebAssembly support, I have a problem using it in Chrome with CSP headers, disallowing unsafe-eval:

Uncaught (in promise) abort({}) at Error [...]

I found out that it crashes at preamble.js:2234 using the abort() function and I suspect that it tries to call the function which cannot be called because the execution is not allowed again. The error is not being thrown in Firefox.

The problem is not that the WebAssembly code was not executed but that the error was thrown without being caught.

Is there anything we can do about that?

HTML example

<html>
  <head>
    <meta http-equiv="Content-Security-Policy" content="default-src 'self'; connect-src 'self' data:; script-src 'self' 'unsafe-inline'">
  </head>
  <body>
    <script>
      window.sodium = {
        onload: function (sodium) {
          let key = sodium.crypto_sign_keypair();
          console.log(key);
        }
      };
    </script>
    <script src="sodium.js" async></script>
  </body>
</html>

[buu700]

I've run into similar issues, and IIRC the root cause is that the Chrome team specifically opted to have unsafe-eval block WebAssembly compilation on the main thread, in the absence of a more suitable wasm-specific CSP directive. This causes libsodium to abort compiling wasm and fall back to asm.js.

sodium should still work (and I just quickly tested and confirmed that onload works for me), but the error logged to the console is kind of annoying. I think the fix on emscripten's end would be to make sure that defining Module.onAbort results in the promise being handled.

[ffflorian]

@buu700 Thank you for the quick reply!

I'd be very happy if you could fix Module.onAbort being handled :slightly_smiling_face:

(if you point me into the right direction, I also might be able to fix it in a PR, would be happy to do it but don't know where to start)

[kripken]

I'm not sure I understand the onAbort issue discussed here, please lets open a new issue for it with more details (or a PR with a fix if someone is already working on that).

[juj]

This looks like great progress. We should not delete the NO_DYNAMIC_EXECUTION variable though, but just make it an ignored no-op (that is, if there's no longer anything that needs to gate on it). This is because projects that utilize it in their build systems would start complaining about an unknown setting variable, and it would then be difficult for a project to support building with old and new compiler versions simultaneously.

Otherwise to be warnings free, projects that wanted NO_DYNAMIC_EXECUTION=1 would have to first do an emcc -v to find which Emscripten compiler they are building against, and then decide whether to apply -s NO_DYNAMIC_EXECUTION=1 or not. So good to silently accept -s NO_DYNAMIC_EXECUTION=0/1 in the future but not make it affect anything.

[nazar-pc]

@juj, It doesn't prevent compilation from happening, doesn't break anything, but makes developers aware that the option is gone and they can drop it if they don't have legacy stuff. It is just a harmless warning, why bother keeping it in code base?

I'd rather add something like -q option for silencing non-critical warnings instead like it is present in many other tools.

[curiousdannii]

I can see merits to both sides there...

At least the useless options should be removed when doing a major release.

[stale[bot]]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant.

[wffurr]

Looks like the only uses for this flag left are in embind (and the run_script function). I'll take a swing at the embind code. It all works without dynamic execution; maybe we can just delete the dynamic execution branches and update the tests.

[stale[bot]]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.