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

Segfault on combo of global ctors, ostream, wasi #11001

Closed
tqchen opened this issue Apr 26, 2020 · 11 comments
Closed

Segfault on combo of global ctors, ostream, wasi #11001

tqchen opened this issue Apr 26, 2020 · 11 comments

Comments

@tqchen
Copy link

tqchen commented Apr 26, 2020

Dear developers. I encountered an interesting case when both global ctors need to be called, and ostream being used in the WASM standalone mode (emsdk latest upstream).

I have a case where I need to call the global static initializers(need to call _start before running other functions), but also have ostream in the code.

What was happening is that when ostream is being used in the code and it somehow triggers certain things to be added to global ctors, and then calling global ctors resulted in a segfault. So far I only get this error on node14, and I am not sure if it is related to the use of WASI.

C++

#include <emscripten.h>
#include <vector>
#include <sstream>

// static intializer, need to call _start
static std::vector<int> x = {1, 2, 3};

extern "C" {
EMSCRIPTEN_KEEPALIVE
int GetX(int i) {
   // use of ostream somehow makes _start fail.
    std::ostringstream os;
    os << "x";
    return x[i];
}
}
wasm_test.wasm: wasm_test.cc
	@mkdir -p $(@D)
	emcc -O3 -std=c++11 -o $@ $<

NodeJS

const { WASI } = require('wasi');

const wasi = new WASI({
    args: process.argv,
    env: process.env
  });

const binary = require('fs').readFileSync('build/wasm_test.wasm');

WebAssembly.instantiate(binary,
    { env: {}, wasi_snapshot_preview1: wasi.wasiImport }).then(({ instance }) => {
  // trigger ctors
  instance.exports._start();
  // test the static vars are correctly initialized.
  console.log(instance.exports.GetX(0));
});
node --experimental-wasi-unstable-preview1  --experimental-wasm-bigint test_wasm.js 
@sbc100
Copy link
Collaborator

sbc100 commented Apr 26, 2020

The the same binary work on wasmer and/or wasmtime? If so that would point to it being a node issue. If not then its more likely and emscripten issue and there is something wrong with the wasm file itself.

@kripken
Copy link
Member

kripken commented Apr 27, 2020

When you say "segfault" do you mean in node.js's code itself, or in the compiled C code (so a wasm trap on out of bounds)?

@tqchen
Copy link
Author

tqchen commented Apr 27, 2020

I think it should be a node problem, the segfault happens at node, here is the backtrace

Thread 1 "node" received signal SIGSEGV, Segmentation fault.
0x0000000001060788 in v8::internal::Runtime::GetObjectProperty(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, bool*) ()
(gdb) bt
#0  0x0000000001060788 in v8::internal::Runtime::GetObjectProperty(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, bool*) ()
#1  0x0000000000ba7b67 in v8::Object::Get(v8::Local<v8::Context>, v8::Local<v8::Value>) ()
#2  0x0000000000ad0f10 in node::wasi::WASI::backingStore(char**, unsigned long*) ()
#3  0x0000000000ad1b71 in node::wasi::WASI::EnvironSizesGet(v8::FunctionCallbackInfo<v8::Value> const&) ()
#4  0x0000000000c02b0b in v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) ()
#5  0x0000000000c040b6 in v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) ()
#6  0x0000000000c04736 in v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) ()
#7  0x00000000013a6339 in Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit ()
#8  0x0000137ee359220a in ?? ()

I also did a hack to fallback to emscripten's import environment provided by the emcc(instead of using the wasi from nodejs) and it works fine.

However I still curious to see what was going on here. Of course this issue can be closed since that may not be an emscripten issue, or it could also due to some incompatibility? I am also curious why iostream and ctor in particular triggers the problem, it would be great to get some of your thoughts, otherwise, please feel free to close the issue.

Thank you!

@sbc100
Copy link
Collaborator

sbc100 commented Apr 27, 2020

Oh, I see the problem. If you want to build portable wasm binaries you need to build with -s STANDALONE_WASM.

The node wasi implementation probably should be crashing though so you still might want to open a bug there. I think its crashing because the wasm module without STANDALONE_WASM does not export memory.

BTW does the crash happen even if you don't call GetX? How about if you don't even call _start? I imagine the crash is happening during _start?

@sbc100
Copy link
Collaborator

sbc100 commented Apr 27, 2020

I managed to reproduce this crash against node 14, even with STANDALONE_WASM.

It looks like it happens with a simple hello world program too:

$ cat test.cc 
#include <stdio.h>

int main() {
  printf("hello\n");
  return 0;
}
$ emcc -o test.wasm test.cc  -s STANDALONE_WASM=1

The result works under wasmer and wasmtime:

$ wasmer test.wasm 
hello
$ wasmtime test.wasm 
hello

But crashes under node14+wasi:

$ cat test.js 
const { WASI } = require('wasi');

const wasi = new WASI({
    args: process.argv,
    env: process.env
  });

const binary = require('fs').readFileSync('test.wasm');

WebAssembly.instantiate(binary,
    { env: {}, wasi_snapshot_preview1: wasi.wasiImport }).then(({ instance }) => {
  // trigger ctors
  console.log("calling start");
  instance.exports._start();
});
$ node-v14.0.0-linux-x64/bin/node --experimental-wasi-unstable-preview1  --experimental-wasm-bigint test.js  --trace-warnings
(node:141678) ExperimentalWarning: WASI is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
calling start
Segmentation fault

@kripken
Copy link
Member

kripken commented Apr 30, 2020

Ok, definitely sounds like a node bug then. @tqchen please open an issue there if you haven't already, and link to it here, thanks!

@tqchen
Copy link
Author

tqchen commented Apr 30, 2020

Nodejs issue nodejs/node#33175

@kripken
Copy link
Member

kripken commented Apr 30, 2020

Thanks @tqchen !

Btw it might be useful to attach the wasm file there, so it's easy for them to reproduce (that's what I'd prefer if I were them, I think, but I could be wrong).

@tqchen
Copy link
Author

tqchen commented May 1, 2020

Thanks @kripken the issue is now confirmed in the nodejs repo and they have updated with a wasm file.

@tqchen
Copy link
Author

tqchen commented May 3, 2020

Update: according to nodejs/node#33175 wasi.start(instance) should be called instead of directly invoking _start

@sbc100
Copy link
Collaborator

sbc100 commented May 3, 2020

Ok that makes sense, and it sounds like they are going to fix the crash too.

Se the bug was in the JS harness code, and nothing to do with emscripten?. Can we close this issue now?

@tqchen tqchen closed this as completed May 3, 2020
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