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

Reading wrong address when memory usage exceeds 2GB #2957

Closed
b-inary opened this issue Jun 20, 2022 · 1 comment · Fixed by #3310
Closed

Reading wrong address when memory usage exceeds 2GB #2957

b-inary opened this issue Jun 20, 2022 · 1 comment · Fixed by #3310
Labels

Comments

@b-inary
Copy link

b-inary commented Jun 20, 2022

Describe the Bug

When memory usage exceeds 2GB, the glue code generated by wasm-pack reads the wrong address.

Steps to Reproduce

The following program uses 3GB of memory.

#[wasm_bindgen]
pub struct HugeData {
    _data1: Vec<u32>,
    data2: Vec<u32>,
}

#[wasm_bindgen]
impl HugeData {
    pub fn new() -> Self {
        let mut _data1 = vec![0; 3 << 27]; // 1.5 GB
        let mut data2 = vec![0; 3 << 27]; // 1.5 GB
        for i in 0..(3 << 27) {
            _data1[i as usize] = i;
            data2[i as usize] = i + (3 << 27);
        }
        Self { _data1, data2 }
    }

    pub fn get_last_element_as_boxed_slice(&self) -> Box<[u32]> {
        self.data2[(3 << 27) - 1..].to_vec().into_boxed_slice()
    }
}

Expected Behavior

get_last_element_as_boxed_slice() should return Uint32Array [ 805306367 ], where 805306367 is equal to (3 << 28) - 1.

Actual Behavior

In my environment, Uint32Array [ 537182218 ] was returned.

The glue code generated by wasm-pack is as follows:

/**
* @returns {Uint32Array}
*/
get_last_element_as_boxed_slice() {
    try {
        const retptr = wasm.__wbindgen_add_to_stack_pointer(-16);
        wasm.hugedata_get_last_element_as_boxed_slice(retptr, this.ptr);
        var r0 = getInt32Memory0()[retptr / 4 + 0];
        var r1 = getInt32Memory0()[retptr / 4 + 1];
        var v0 = getArrayU32FromWasm0(r0, r1).slice();
        wasm.__wbindgen_free(r0, r1 * 4);
        return v0;
    } finally {
        wasm.__wbindgen_add_to_stack_pointer(16);
    }
}

Here, var r0 is a signed 32-bit integer, and it is negative when memory usage exceeds 2GB. Therefore, getArrayU32FromWasm0(r0, r1) reads memory of negative address, which causes this bug.

@b-inary b-inary added the bug label Jun 20, 2022
@alexcrichton
Copy link
Contributor

Thanks for the report! I think the generated JS hasn't ever been audited for pointers >= 2GB unfortunately. In theory this can be solved by sprinkling some >>> 0 additions in the code generation but I don't know how to best handle make sure everything is covered.

kristoff3r added a commit to kristoff3r/wasm-bindgen that referenced this issue Feb 15, 2023
Pointers being passed from WASM to JS are interpreted as signed, so
large pointers ended up negative. This prevented WASM programs from
allocating more than 2GB.

To fix it we make pointer use unsigned (via >>> 0) for all malloc/realloc
calls, and for all pointer arguments while generating the shim functions.
Ideally there would be an abstraction that guarantees we do it in all
cases, but that would require a major refactor.

To test it we use gg-alloc as the global allocator while running tests.
This allocator was made specifically for testing this issue, but was
never upstreamed. It only allocates memory above 2GiB.

Fixes rustwasm#2388
Fixes rustwasm#2957
kristoff3r added a commit to kristoff3r/wasm-bindgen that referenced this issue Feb 15, 2023
Pointers being passed from WASM to JS are interpreted as signed, so
large pointers ended up negative. This prevented WASM programs from
allocating more than 2GB.

To fix it we make pointer use unsigned (via >>> 0) for all malloc/realloc
calls, and for all pointer arguments while generating the shim functions.
Ideally there would be an abstraction that guarantees we do it in all
cases, but that would require a major refactor.

To test it we use gg-alloc as the global allocator while running tests.
This allocator was made specifically for testing this issue, but was
never upstreamed. It only allocates memory above 2GiB.

Fixes rustwasm#2388
Fixes rustwasm#2957
kristoff3r added a commit to kristoff3r/wasm-bindgen that referenced this issue Feb 15, 2023
Pointers being passed from WASM to JS are interpreted as signed, so
large pointers ended up negative. This prevented WASM programs from
allocating more than 2GB.

To fix it we make pointer use unsigned (via >>> 0) for all malloc/realloc
calls, and for all pointer arguments while generating the shim functions.
Ideally there would be an abstraction that guarantees we do it in all
cases, but that would require a major refactor.

To test it we use gg-alloc as the global allocator while running tests.
This allocator was made specifically for testing this issue, but was
never upstreamed. It only allocates memory above 2GiB.

Fixes rustwasm#2388
Fixes rustwasm#2957
kristoff3r added a commit to kristoff3r/wasm-bindgen that referenced this issue Feb 17, 2023
Pointers being passed from WASM to JS are interpreted as signed, so
large pointers ended up negative. This prevented WASM programs from
allocating more than 2GB.

To fix it we make all pointers unsigned (via >>> 0) for all malloc/realloc
calls and inside shim functions. Ideally we would have an abstraction
that guarantees we catch all cases, but that would require a major
refactor.

To test it we add gg-alloc as an optional system allocator for
wasm-bindgen-test. It only allocates in the upper 2GB range and was made to
test this exact issue but was never upstreamed.

Fixes rustwasm#2388
Fixes rustwasm#2957
alexcrichton pushed a commit that referenced this issue Feb 17, 2023
Pointers being passed from WASM to JS are interpreted as signed, so
large pointers ended up negative. This prevented WASM programs from
allocating more than 2GB.

To fix it we make all pointers unsigned (via >>> 0) for all malloc/realloc
calls and inside shim functions. Ideally we would have an abstraction
that guarantees we catch all cases, but that would require a major
refactor.

To test it we add gg-alloc as an optional system allocator for
wasm-bindgen-test. It only allocates in the upper 2GB range and was made to
test this exact issue but was never upstreamed.

Fixes #2388
Fixes #2957
emilk pushed a commit to rerun-io/wasm-bindgen that referenced this issue Mar 17, 2023
Pointers being passed from WASM to JS are interpreted as signed, so
large pointers ended up negative. This prevented WASM programs from
allocating more than 2GB.

To fix it we make all pointers unsigned (via >>> 0) for all malloc/realloc
calls and inside shim functions. Ideally we would have an abstraction
that guarantees we catch all cases, but that would require a major
refactor.

To test it we add gg-alloc as an optional system allocator for
wasm-bindgen-test. It only allocates in the upper 2GB range and was made to
test this exact issue but was never upstreamed.

Fixes rustwasm#2388
Fixes rustwasm#2957
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants