justjake / quickjs-emscripten

Safely execute untrusted Javascript in your Javascript, and execute synchronous code that uses async functions
https://www.npmjs.com/package/quickjs-emscripten
Other
1.27k stars 95 forks source link

`Out of bounds memory access` Error only on ARM Safari #166

Closed yar2001 closed 3 months ago

yar2001 commented 6 months ago

I encountered a strange issue: when running a code that includes a negative number smaller than -2 on Safari, it will throw Out of bounds memory access (evaluating 'a.apply(null,h)') Error.

The following code will work on all platforms except iOS or Mac ARM Safari.

   const QuickJS = await getQuickJS(); // 0.29.1

    try {
      for (let i = 0; i < 2000; i++) {
        const runtime = QuickJS.newRuntime();
        const context = runtime.newContext();

        const result = context.evalCode(`-2`);

        context.unwrapResult(result).dispose();
        context.dispose();
        runtime.dispose();
      }
      console.log('ok');
    } catch (err) {
      console.error(err);
    }

To get the error, the code should be executed multiple times. In a real-world scenario, with longer code, the error will appear sooner.

I looked at a related issue but did not solve the problem: https://github.com/justjake/quickjs-emscripten/issues/35

Here is the replication:

quickjs-emscripten-ios-error.zip

Works on Chrome: Screenshot from 2024-03-24 23-22-08

Mac M2 Safari 17.4 throws the error: Screenshot from 2024-03-24 23-21-42

justjake commented 6 months ago

Thanks for the report. Does it need to be x < -128? Because to me the creation of many runtime/context instances seems more likely to be the cause. I wonder if Safari is hitting some limit with memory allocation so we end up with a runtime or context pointer that’s out of bounds for some reason.

justjake commented 6 months ago

Does the error occur with other build variants?

yar2001 commented 6 months ago

Does it need to be x < -128?

Yes, it needs to be a negative number smaller than -2. The code will throw the error if it is -2, -3, -128...

My original scenario is a long piece of code that executes 60 times per second. There will be no error if I remove the negative number from the code. A strange behavior that I took a while to find.

Does the error occur with other build variants?

Yes, it also occurs on quickjs-ng.

Matthiee commented 5 months ago

Hi, we have encountered the same issue in our app on Safari.

The issue is somehow related to negative numbers.

Additional details

Browserstack testing

I was able to test and reproduce the issue on the following environments.

Variants

I couldn't reproduce the issue on asyncify builds.

Variant Works
@jitl/quickjs-wasmfile-release-sync :x:
@jitl/quickjs-wasmfile-release-asyncify :white_check_mark:
@jitl/quickjs-ng-wasmfile-release-sync :x:
@jitl/quickjs-ng-wasmfile-release-asyncify :white_check_mark:
@jitl/quickjs-singlefile-browser-release-sync :x:
@jitl/quickjs-singlefile-browser-release-asyncify :white_check_mark:

Repro

We managed to get the out of bounds memory access by using a single context.

  const context = QuickJS.newContext();
  for (let i = 0; i < 1000000; i++) {
    const result = context.evalCode(`1 < -128`);

    context.unwrapResult(result).dispose();
  }
  context.dispose();
  console.log("ok");
yar2001 commented 5 months ago

I tested it again and found that any negative integer literal that is less than -2 would throw the error. Error: -2 -(2) -2.0 -0b10 1 < -2 OK: -1 -0 +2 -2.1 0 - 2 -(2 + 0) var x = 2; -x -0b1

I guess the problem may occur in js_atof function (quickjs.c#L10189) ?

past-due commented 5 months ago

We don't use this project directly, but we do use QuickJS embedded into a larger C++ project which is compiled to WASM via Emscripten.

If this is the same issue that I've been digging into, it seems it may have something to do with push_short_int() and the OP_push_i8 and OP_push_i16 SHORT_OPCODES.

Possible workaround:

yar2001 commented 5 months ago

We don't use this project directly, but we do use QuickJS embedded into a larger C++ project which is compiled to WASM via Emscripten.

If this is the same issue that I've been digging into, it seems it may have something to do with push_short_int() and the OP_push_i8 and OP_push_i16 SHORT_OPCODES.

Thanks a lot! Based on my testing, it will fix the problem.

This seems to be an upstream library issue, should we report it to QuickJS, Emscripten or Webkit?

past-due commented 5 months ago

Thanks a lot! Based on my testing, it will fix the problem.

This seems to be an upstream library issue, should we report it to QuickJS, Emscripten or Webkit?

I am only able to reproduce this on Safari (never had an issue on Chrome or Firefox), so it seems like a WebKit-specific issue.

As such, I’d suggest reporting it to Emscripten (who might have some further ideas as to what might be going wrong) and WebKit.

past-due commented 5 months ago

Something important to note: This issue is only reproducible on Safari / WebKit on Apple Silicon / ARM. The issue does not occur on Safari on Intel Macs. So it looks to be a WebKit + ARM / Apple Silicon specific issue.

yar2001 commented 5 months ago

I'm not sure if I can provide an informative enough issue for Emscripten, can @justjake help? Probably we should report the Out of bounds memory access error that is caused by OP_push_i8 and OP_push_i16 SHORT_OPCODES in push_short_int() only on ARM Safari. Thanks a lot!