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

BigUint64Array some large numbers broken in wasm32-wasi #202

Closed
lastmjs opened this issue Nov 30, 2023 · 5 comments
Closed

BigUint64Array some large numbers broken in wasm32-wasi #202

lastmjs opened this issue Nov 30, 2023 · 5 comments

Comments

@lastmjs
Copy link

lastmjs commented Nov 30, 2023

Here's a very simple reproducible piece of code that shows the issue:

// This should print an element with value 18446600000000000000n
// It actually prints an element with value 18446740737488355328n
const bytes = [0, 128, 184, 57, 247, 124, 255, 255];
const buffer = new ArrayBuffer(8);
const uint8Array = new Uint8Array(buffer);
uint8Array.set(bytes);
const bigUint64Array = new BigUint64Array(buffer);
console.log(bigUint64Array[0].toString());

Some large numbers relatively close to the maximum size of an unsigned 64 bit integer (2^64 - 1 or 18_446_744_073_709_551_615) are not able to be put into the new BigUint64Array constructor. The resulting BigUint64Array has a number that is not the same as the number being passed into the constructor.

The code above returns the correct number 18446600000000000000n on Node.js, Chrome, QuickJS compiled to my Ubuntu machine, Javy running on my Ubuntu machine, and rquickjs running on my Linux machine. Javy compiled to wasm32-wasi and rquickjs compiled to wasm32-wasi both return the incorrect number 18446740737488355328n. I've used both wasmtime and wasmer to test that the incorrect number is returned.

Other numbers are broken as well.

@lastmjs lastmjs changed the title BigUint64Array large numbers broken in wasm32-wasi BigUint64Array some large numbers broken in wasm32-wasi Nov 30, 2023
@lastmjs
Copy link
Author

lastmjs commented Nov 30, 2023

Here's some more interesting code that creates the same BigUint64Array differently. As soon as the number is assigned into the BigUint64Array the damage is done. bigIntValue is correct the whole time, it's when we read the first element of the BigUint64Array that the number appears to becomes corrupted:

const bytes = [0, 128, 184, 57, 247, 124, 255, 255];

let bigIntValue = BigInt(0);
for (let i = 0; i < bytes.length; i++) {
    bigIntValue =
        (bigIntValue << BigInt(8)) +
        BigInt(bytes[bytes.length - 1 - i]);
}

const bigUint64Array = new BigUint64Array(1);
bigUint64Array[0] = bigIntValue;
console.log(bigUint64Array[0].toString());

@ulan
Copy link

ulan commented Dec 1, 2023

I narrowed down the problem to this line in code: https://github.com/bellard/quickjs/blob/2788d71e823b522b178db3b3660ce93689534e6d/quickjs.c#L12031

Something goes wrong after the call to bf_set_ui(a, v).

Google search for that function shows: #131

If I apply the fix of that issue locally, then the problem disappears: #132

@bellard
Copy link
Owner

bellard commented Dec 1, 2023

fixed

@bellard bellard closed this as completed Dec 1, 2023
@ulan
Copy link

ulan commented Dec 1, 2023

@bellard: thanks a lot for pushing the fix. There are other occurrences of LIMB_BITS - shift that potentially have similar issues. Here is a patch I was testing locally: ulan/javy@984b8d1

Do you think those places are safe or should also be fixed?

@bellard
Copy link
Owner

bellard commented Dec 1, 2023

Normally there is a check before. Adding unnecessary tests is expensive and should be avoided.

TooTallNate pushed a commit to TooTallNate/quickjs that referenced this issue Dec 18, 2023
Unclear why sending a SIGQUIT signal sometimes works and sometimes
doesn't but it's probably some kind of race condition in Cygwin's
emulation layer.

Fixes: quickjs-ng/quickjs#184
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants