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

worker_threads: multiple crashes caused by duplicate objects in transferList #25786

Closed
chjj opened this issue Jan 29, 2019 · 1 comment
Closed
Labels
worker Issues and PRs related to Worker support.

Comments

@chjj
Copy link
Contributor

chjj commented Jan 29, 2019

  • Version: v11.8.0
  • Platform: Arch Linux 4.20.4-arch1-1-ARCH deps: update openssl to 1.0.1j #1 SMP PREEMPT Wed Jan 23 00:12:22 UTC 2019 x86_64 GNU/Linux
  • Subsystem: worker_threads

I'm currently creating a worker_threads compatability library which seeks to replicate worker_threads behavior as accurately as possible using the child_process module. This has caused me to investigate a number of edge cases with the worker_threads module.

My own implementation question was something like this:

worker.postMessage(port1, [port1, port1]);

What should happen? Should postMessage throw or simply ignore the second entry in the transfer list? It turns out node.js does neither.

Example case:

const threads = require('worker_threads');

if (threads.isMainThread) {
  const worker = new threads.Worker(__filename);
  const {port1, port2} = new threads.MessageChannel();

  port1.on('message', console.log);

  // Causes a segfault:
  worker.postMessage(port2, [port2, port2]);

  // Causes a fatal error:
  const buf = new Uint8Array(1);
  worker.postMessage(buf, [buf.buffer, buf.buffer]);
} else {
  threads.parentPort.on('message', (port) => {
    if (port instanceof threads.MessagePort)
      port.postMessage('hello');
  });
}

It seems that a duplicate port in the transfer list causes a segfault (I'm sorry I don't currently have a debug build available to provide a stack trace).

Transferring a duplicate array buffer causes a fatal error (at least a bit more descriptive):

FATAL ERROR: v8_ArrayBuffer_Externalize ArrayBuffer already externalized
 1: 0x55fefb3fe881 node::Abort() [node]
 2: 0x55fefb400238 node::OnFatalError(char const*, char const*) [node]
 3: 0x55fefb5917cb v8::Utils::ReportApiFailure(char const*, char const*) [node]
 4: 0x55fefb5af63b v8::ArrayBuffer::Externalize() [node]
 5: 0x55fefb448747 node::worker::Message::Serialize(node::Environment*, v8::Local<v8::Context>, v8::Local<v8::Value>, v8::Local<v8::Value>, v8::Local<v8::Object>) [node]
 6: 0x55fefb44923f node::worker::MessagePort::PostMessage(node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Value>) [node]
 7: 0x55fefb449608 node::worker::MessagePort::PostMessage(v8::FunctionCallbackInfo<v8::Value> const&) [node]
 8: 0x55fefb606dcf  [node]
 9: 0x55fefb608307  [node]
10: 0x5ac0f1cfc5d
Aborted (core dumped)

Update:

After some more investigating, it looks like the browser throws a DataCloneError in both of these edge cases.

Duplicate port:

Uncaught DataCloneError: Failed to execute 'postMessage' on 'Worker': Message port at index 1 is a duplicate of an earlier port.

Duplicate array buffer:

Uncaught DataCloneError: Failed to execute 'postMessage' on 'Worker': ArrayBuffer at index 1 is a duplicate of an earlier ArrayBuffer.
@Fishrock123 Fishrock123 added the worker Issues and PRs related to Worker support. label Jan 29, 2019
addaleax added a commit to addaleax/node that referenced this issue Jan 30, 2019
Throw a `DataCloneError` exception when encountering duplicate
`ArrayBuffer`s or `MessagePort`s in the transfer list.

Fixes: nodejs#25786
@addaleax
Copy link
Member

#25815 should address this – thanks for the issue, and please keep reporting any issues you run into with Workers! :)

addaleax added a commit that referenced this issue Feb 1, 2019
Throw a `DataCloneError` exception when encountering duplicate
`ArrayBuffer`s or `MessagePort`s in the transfer list.

Fixes: #25786

PR-URL: #25815
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants