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 when using SharedArrayBuffer from an unrefed worker thread #28777

Closed
devongovett opened this issue Jul 20, 2019 · 2 comments
Closed
Assignees
Labels
confirmed-bug Issues with confirmed bugs. worker Issues and PRs related to Worker support.

Comments

@devongovett
Copy link
Contributor

devongovett commented Jul 20, 2019

  • Version: 12.6.0
  • Platform: macOS
  • Subsystem: worker_threads

The following code causes a segfault:

const {Worker} = require('worker_threads');

const w = new Worker(`
  const { parentPort } = require('worker_threads');
  parentPort.on('message', () => {
    const sharedArrayBuffer = new SharedArrayBuffer(12);
    parentPort.postMessage(sharedArrayBuffer);
  });
`, { eval: true });
w.unref();
w.once('message', () => {
 console.log('done');
});
w.postMessage('go');

The segfault occurs due to usage of unref() on the worker. Without that call, the process exits as normal (when process.exit() is called from within the message handler, otherwise it keeps running).

Backtrace from lldb:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x20)
  * frame #0: 0x00000001001947dd node`v8::ArrayBufferDeleter(void*, unsigned long, void*) + 13
    frame #1: 0x00000001000ed298 node`node::worker::SharedArrayBufferMetadata::~SharedArrayBufferMetadata() + 24
    frame #2: 0x00000001000ed55d node`std::__1::__shared_ptr_pointer<node::worker::SharedArrayBufferMetadata*, std::__1::default_delete<node::worker::SharedArrayBufferMetadata>, std::__1::allocator<node::worker::SharedArrayBufferMetadata> >::__on_zero_shared() + 23
    frame #3: 0x00007fff6cc7fd42 libc++.1.dylib`std::__1::__shared_weak_count::__release_shared() + 40
    frame #4: 0x00000001000ed453 node`node::worker::(anonymous namespace)::SABLifetimePartner::~SABLifetimePartner() + 33
    frame #5: 0x00000001000387d4 node`node::Environment::RunCleanup() + 164
    frame #6: 0x00000001000ac686 node`node::NodeMainInstance::Run() + 658
    frame #7: 0x0000000100056ccc node`node::Start(int, char**) + 237
    frame #8: 0x00007fff6fa4a3d5 libdyld.dylib`start + 1
    frame #9: 0x00007fff6fa4a3d5 libdyld.dylib`start + 1

The cause (within node) appears to be in the destructor of SharedArrayBufferMetadata:

SharedArrayBufferMetadata::~SharedArrayBufferMetadata() {
contents_.Deleter()(contents_.Data(),
contents_.ByteLength(),
contents_.DeleterData());
}

Possibly this is a race between the main thread and worker thread, but I'm not familiar enough with node's internals to debug further.

@Trott Trott added the worker Issues and PRs related to Worker support. label Jul 20, 2019
@addaleax addaleax added the confirmed-bug Issues with confirmed bugs. label Jul 20, 2019
@addaleax addaleax self-assigned this Jul 20, 2019
@addaleax
Copy link
Member

Thank you for the excellent reproduction! #28788 should address this.

@devongovett
Copy link
Contributor Author

Thanks for the quick fix!

addaleax added a commit to addaleax/node that referenced this issue Jul 21, 2019
Use the parent thread’s `ArrayBuffer::Allocator` when creating a
Worker instance, as that allocator is guaranteed to outlive the
Worker itself.

This requires making the zero-fill flag a thread_local variable
in order to avoid race conditions between different threads.
A test for that behaviour is added as well.

Fixes: nodejs#28777
Fixes: nodejs#28773
addaleax added a commit to addaleax/node that referenced this issue Aug 17, 2019
Keep a reference to the `ArrayBuffer::Allocator` alive for at least
as long as a `SharedArrayBuffer` allocated by it lives.

Refs: nodejs#28788
Fixes: nodejs#28777
Fixes: nodejs#28773
targos pushed a commit that referenced this issue Aug 20, 2019
Keep a reference to the `ArrayBuffer::Allocator` alive for at least
as long as a `SharedArrayBuffer` allocated by it lives.

Refs: #28788
Fixes: #28777
Fixes: #28773

PR-URL: #29190
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. worker Issues and PRs related to Worker support.
Projects
None yet
3 participants