Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
yar2001 opened this issue Mar 24, 2024 · 10 comments · Fixed by #179
Closed

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

yar2001 opened this issue Mar 24, 2024 · 10 comments · Fixed by #179
Labels
bug Something isn't working

Comments

@yar2001
Copy link
Contributor

yar2001 commented Mar 24, 2024

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: #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
Copy link
Owner

justjake commented Mar 25, 2024

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
Copy link
Owner

Does the error occur with other build variants?

@yar2001
Copy link
Contributor Author

yar2001 commented Mar 28, 2024

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
Copy link

Matthiee commented Apr 9, 2024

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.

  • Sonoma - Safari 17.3
  • Ventura - Safari 16.5
  • Monterey - Safari 15.6
  • Big Sur - Safari 14.1

Variants

I couldn't reproduce the issue on asyncify builds.

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

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
Copy link
Contributor Author

yar2001 commented Apr 14, 2024

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
Copy link

past-due commented Apr 17, 2024

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:

  • Modifying push_short_int() as follows seems to avoid the crash on Safari (at least in some very initial testing in my own particular use-case - I have not tested with the repro above, as we don't use this project directly):
  static void push_short_int(DynBuf *bc_out, int val)
  {
  #if SHORT_OPCODES
      if (val >= -1 && val <= 7) {
          dbuf_putc(bc_out, OP_push_0 + val);
          return;
      }
+ # if !defined(__EMSCRIPTEN__)
      if (val == (int8_t)val) {
          dbuf_putc(bc_out, OP_push_i8);
          dbuf_putc(bc_out, val);
          return;
      }
      if (val == (int16_t)val) {
          dbuf_putc(bc_out, OP_push_i16);
          dbuf_put_u16(bc_out, val);
          return;
      }
+ # endif
  #endif
      dbuf_putc(bc_out, OP_push_i32);
      dbuf_put_u32(bc_out, val);
  }

@yar2001
Copy link
Contributor Author

yar2001 commented Apr 18, 2024

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
Copy link

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
Copy link

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
Copy link
Contributor Author

yar2001 commented Apr 19, 2024

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!

@yar2001 yar2001 changed the title Out of bounds memory access Error on Safari Out of bounds memory access Error only on ARM Safari Apr 19, 2024
@justjake justjake added the bug Something isn't working label Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants