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

Memory corruption when compiling dylink + pthreads + exceptions #13398

Closed
jprendes opened this issue Feb 1, 2021 · 2 comments · Fixed by #16314 · May be fixed by #13447
Closed

Memory corruption when compiling dylink + pthreads + exceptions #13398

jprendes opened this issue Feb 1, 2021 · 2 comments · Fixed by #16314 · May be fixed by #13447

Comments

@jprendes
Copy link
Contributor

jprendes commented Feb 1, 2021

The following example seems to constantly result in a memory corruption.
This happens regardless of whether I am actually spinning a new thread or not (as in the example).

main.cpp

#include <stdio.h>
int get_value(char c); // from side module
int main(void) {
    printf("%d\n", get_value('x'));
    return 0;
}

side.cpp

#include <string>
int get_value(char x) {
    std::string value = "123";
    value.push_back(x);
    return value.length();
}

I am compiling with

em++ -s ASSERTIONS=1 -g -pthread -Wno-experimental -s DISABLE_EXCEPTION_CATCHING=0 -s SIDE_MODULE=1 -o side.wasm side.cpp
em++ -s ASSERTIONS=1 -g -pthread -Wno-experimental -s DISABLE_EXCEPTION_CATCHING=0 -s MAIN_MODULE=1 -s EXIT_RUNTIME=1 -s RUNTIME_LINKED_LIBS=['side.wasm'] -o main.html main.cpp

The example correctly prints 4, but then it is followed by

Uncaught RuntimeError: abort(Runtime error: The application has corrupted its heap memory area (address zero)!) at Error
    at jsStackTrace (http://127.0.0.1:8080/main.js:2201:19)
    at stackTrace (http://127.0.0.1:8080/main.js:2751:16)
    at abort (http://127.0.0.1:8080/main.js:1567:44)
    at checkStackCookie (http://127.0.0.1:8080/main.js:1339:46)
    at exitRuntime (http://127.0.0.1:8080/main.js:1405:3)
    at exit (http://127.0.0.1:8080/main.js:54481:5)
    at callMain (http://127.0.0.1:8080/main.js:54338:7)
    ...
@jprendes
Copy link
Contributor Author

jprendes commented Feb 1, 2021

This seems to happen when a noexcept function calls a function that "could" throw. This seems to be the case with musl's allocators, used by std::string in my previous example.

Here's an example:

main.cpp

#include <stdio.h>
int get_value();
int main(void) {
    printf("%d\n", get_value());
    return 0;
}

side.cpp

#include <stdlib.h>
int some_function() noexcept {
    return atoi("42");
}
int get_value() {
    return some_function();
}

And here's another example that doesn't require any library:

main.cpp

int random() {
    return 42;
}
int get_value();
int main(void) {
    return get_value();
}

side.cpp

int random();
int some_function() noexcept {
    return random();
}
int get_value() {
    return some_function();
}

In both cases removing the noexcept everything works as expected.
Both examples are compiled with the same commands from the previous post.

It looks like the compiler is generating code that required thread local storage, but doesn't actually allocate any when the thread starts.

In both cases the generated side.wasm file contains:

  (global $__tls_base (;3;) (mut i32) (i32.const 0))
  (global $__tls_size (;4;) (export "__tls_size") i32 (i32.const 0))
  (global $__tls_align (;5;) (export "__tls_align") i32 (i32.const 0))
  ...
  (func $__wasm_init_tls (;2;) (export "__wasm_init_tls") (param $var0 i32)
  )

the module claims to require 0 bytes of tls, and tls init is actually empty, where normally it would assign __tls_base and initialize the memory.

In both cases the definition of some_function starts as follows, where __tls_base is not initialized and has a value of 0.

  (func $_Z13some_functionv (;9;) (export "_Z13some_functionv") (result i32)
    (local $var0 i32) (local $var1 i32) (local $var2 i32) (local $var3 i32) (local $var4 i32) (local $var5 i32) (local $var6 i32) (local $var7 i32) (local $var8 i32) (local $var9 i32) (local $var10 i32) (local $var11 i32) (local $var12 i32) (local $var13 i32) (local $var14 i32)
    i32.const 0
    local.set $var0
    global.get $__tls_base
    local.set $var1
    local.get $var1
    local.get $var0
    i32.add
    local.set $var2
    i32.const 0
    local.set $var3
    local.get $var2
    local.get $var3
    i32.store

It is this last store in the listing that corrupts the memory cookie.

@jprendes
Copy link
Contributor Author

jprendes commented Feb 4, 2021

Ok, it turns out that exceptions + pthread shares TLS data addresses with JS, which is mentioned as a missing feature in @sbc100 's initial commit for dylink + pthread.

Adding TLS support and making sure that side modules are linked against emscripten's exceptions builtin code should fix this issue.

sbc100 added a commit that referenced this issue Feb 16, 2022
The remaining issue with exception with this combination of features
was fixed by this upstream llvm change: https://reviews.llvm.org/D119630

Fixes: #13398, #13447
sbc100 added a commit that referenced this issue Feb 16, 2022
The remaining issue with exception with this combination of features
was fixed by this upstream llvm change: https://reviews.llvm.org/D119630

Fixes: #13398, #13447
sbc100 added a commit that referenced this issue Feb 16, 2022
The remaining issue with exception with this combination of features
was fixed by this upstream llvm change: https://reviews.llvm.org/D119630

Fixes: #13398, #13447
sbc100 added a commit to llvm/llvm-project that referenced this issue Feb 16, 2022
Since the code for apply data relocations can sometimes use
the values stored in he globals, they need to be relocated
before the data relocations can be run.

Fixes: emscripten-core/emscripten#13398

Differential Revision: https://reviews.llvm.org/D119666
sbc100 added a commit that referenced this issue Feb 17, 2022
The remaining issue with exceptions and this combination of features
were fixed by https://reviews.llvm.org/D119630 and
https://reviews.llvm.org/D119666.

Fixes: #13398, #13447
sbc100 added a commit that referenced this issue Feb 17, 2022
The remaining issue with exceptions and this combination of features
were fixed by https://reviews.llvm.org/D119630 and
https://reviews.llvm.org/D119666.

Fixes: #13398, #13447
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
Since the code for apply data relocations can sometimes use
the values stored in he globals, they need to be relocated
before the data relocations can be run.

Fixes: emscripten-core/emscripten#13398

Differential Revision: https://reviews.llvm.org/D119666
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant