fix: convert result of allocate_mappings from signed to unsigned

[nornagon]

I haven't yet tested this because I don't have a good repro locally, but this might fix #412? I noticed that the rust bundle returns a signed integer, and I'm seeing it sometimes come back as negative (i.e. > 2GB). I think this maybe started being an issue with https://v8.dev/blog/4gb-wasm-memory.

[ochameau]

Having a way to reproduce would be super helpful, even if this include setting up some other project.

[nornagon]

I think that in order to reproduce this, you need to create enough SourceMapConsumers that the WASM memory exceeds 2GB. I'll see if I can write a test.

[nornagon]

@ochameau i've added a somewhat crude test that reproduces the behavior. However, it seems to crash with "unreachable" instead of the RangeError when I put in this fix. I'm not sure exactly why that would be. But at least there's a reliable way to produce the error described in #412 now!

[nornagon]

ah, rebuilt mappings.wasm with -g and it looks like the unreachable is an allocation error:

RuntimeError: unreachable
    at alloc::alloc::handle_alloc_error::h4d4fb786473fa992 (<anonymous>:wasm-function[54]:0xb65c)
    at parse_mappings (<anonymous>:wasm-function[23]:0xae24)

[nornagon]

figured it out, my test for "are we over 2GB yet" was wrong, and the "unreachable" was happening when the 4GB limit was reached. the test is updated now to check the size of the WASM memory exceeds 3GB. Experimentally, exceeding 2GB wasn't enough because the mappings weren't being allocated past the 2GB byte mark. The test as written now fails without the fix, and passes with the fix.

[ochameau]

Thanks a ton for being able to have a test for this!

Now, I'm wondering about the actual fix.
I'm new to Rust and Wasm so I don't know exactly what is going on.
It looks like once we reach some particular memory size, the address returned by rust's allocate_mapping function gets out of hands. This is typed as *mut u8. This isn't clear what it means from the Javascript point of view...

Your test highlights that the returned value starts being negative from the Javascript codebase around 2GB (i.e. 2 * 1024 * 1024 * 1024).
Why would that suddently happen? Is there really a 2GB limit for Wasm memory? If so why exports.memory.buffer.byteLength is able to go above this limit?
Also Javascript numbers support larger numbers. Why does that start being negative when going over 2GB? Could there be a typedef issue in rust code? Or is there something wrong in the convertion of *mut u8 to JS value?

I'm wondering if using unsigned right shift is actually correct. It sounds like we may just map to some arbitrary address, that is no longer negative, so it no longer throw. But it may not necessarily be the address we meant.

[nornagon]

It’s because the return type of allocate_mappings is i32 in the compiled wasm, which is a *signed* 32 bit integer, i.e. 31 bits of int plus one bit saying whether the number is positive or negative. 2 GB is the largest positive integer that can be represented in an i32, so once it gets above that it wraps around to negative. The right shift “reinterprets” the number as *unsigned*, which is what the original rust type was (you can’t have a negative pointer).

On Tue, Nov 22, 2022 at 06:55 ochameau ***@***.***> wrote: Thanks a ton for being able to have a test for this! Now, I'm wondering about the actual fix. I'm new to Rust and Wasm so I don't know exactly what is going on. It looks like once we reach some particular memory size, the address returned by rust's allocate_mapping function gets out of hands. This is typed as *mut u8. This isn't clear what it means from the Javascript point of view... Your test highlights that the returned value starts being negative from the Javascript codebase around 2GB (i.e. 2 * 1024 * 1024 * 1024). Why would that suddently happen? Is there really a 2GB limit for Wasm memory? If so why exports.memory.buffer.byteLength is able to go above this limit? Also Javascript numbers support larger numbers. Why does that start being negative when going over 2GB? Could there be a typedef issue in rust code? Or is there something wrong in the convertion of *mut u8 to JS value? I'm wondering if using unsigned right shift is actually correct. It sounds like we may just map to some arbitrary address, that is no longer negative, so it no longer throw. But it may not necessarily be the address we meant. — Reply to this email directly, view it on GitHub <#473 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABKGACY3JHKKO7H26HJRWTWJTNE3ANCNFSM6AAAAAASC4IKHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- j

[nornagon]

Ah, I think CI might be hitting "unreachable" because possibly the runner has less RAM, so V8 chooses a smaller heap limit...?

[ochameau]

Thanks a lot for the explanation.

It makes sense. I'm really surprised there isn't any intermediate layer to do that automatically.
It looks like we would benefit from using wasm-bindgen. I tried migrating to it. It isn't obvious, but it doesn't seem to apply the >>> 0 trick to allocate_mappings, whereas it does in many places...
rustwasm/wasm-bindgen#1401
rustwasm/wasm-bindgen#1388

There is still one suspicous behavior. This may be unrelated but may highlight some other leak.
When I log mappingBufPtr while running your test, I see that it overflows only once on 2816143240.
Without your patch, this value was negative and it throws on UInt8Array.
With your patch, the value is the one I mention and code keeps running.
The one thing that puzzles me is that later on, we get a lower value, 1114192 which used to be used earlier for that same pointer.

You can see that with the following patch:

diff --git a/lib/source-map-consumer.js b/lib/source-map-consumer.js
index 4c9fbd3..ba58832 100644
--- a/lib/source-map-consumer.js
+++ b/lib/source-map-consumer.js
@@ -302,6 +302,11 @@ class BasicSourceMapConsumer extends SourceMapConsumer {
     const size = aStr.length;
 
     const mappingsBufPtr = this._wasm.exports.allocate_mappings(size) >>> 0;
+    if (globalThis.lastPtr != mappingsBufPtr) {
+      console.log(mappingsBufPtr)
+      globalThis.lastPtr = mappingsBufPtr;
+    }
+
     const mappingsBuf = new Uint8Array(
       this._wasm.exports.memory.buffer,
       mappingsBufPtr,

$ yarn test
yarn run v1.21.1
$ node test/run-tests.js
1114128
1114160
1114192
1119512
1114192
1114224
1114192
1114224
1114192
1114224
1114192
1119512
1114192
1114224
1114192
1114224
1114192
1114224
1114192
1118528
4768120
1410455680
2816143240
1114192
1114224
1114192

Why is there such an increase in memory and then we are able to allocate back in lower addresses?
Is that just expected entropy?

Anyway, feel free to ignore. I think your patch is super valuable as-is.
Could you just add a comment to explain the >>>0 ?
Regarding the test, while it was super handy to investigate, I'm not sure it is worth having in-tree.
It makes the overall test suite much slower to run...

[nornagon]

@ochameau i think that's because the memory gets freed when the SourceMapConsumer is destroyed. Once the memory is freed, the allocator can use the lower addresses again.

[nornagon]

BTW, we don't need to also do this for _mappingsPtr. It's fine for that one to be negative because we only ever use it to pass it back into wasm, which will correctly interpret the value as unsigned. We need it for mappingsBufPtr because we use it to index into a Uint8Array, i.e. the WASM memory object.

Agree that converting to wasm-bindgen would be a good idea!

[ochameau]

Sorry for the delay, I missed the notification about your newest revision...

Thanks a lot for having looked into this!

[alizubair76]

@nornagon , @ochameau When can we expect the release of this fix?