Segfault with unref on a worker with ArrayBuffer in `transferList`

[timsuchanek]

• Version: v14.2.0
• Platform: mac OS 10.13.6

What steps will reproduce the bug?

When communicating a Uint8 Array Buffer from a worker to the parent process with postMessage, which is included in the transferList argument and then calling unref on the worker, I get a Segfault: 'node index.js' terminated by signal SIGSEGV (Address boundary error).

index.js

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

const worker = new Worker(path.join(__dirname, 'worker.js'))
worker.postMessage({})
worker.on('message', (message) => {
  const hash = Buffer.from(message.value).toString('hex')
  console.log(hash)
  worker.unref()
})

worker.js

const fs = require('fs')
const crypto = require('crypto')
const { parentPort } = require('worker_threads')

parentPort.on('message', (message) => {
  const hasher = crypto.createHash('sha256')
  fs.createReadStream('example.txt')
    .pipe(hasher)
    .on('finish', () => {
      const { buffer } = hasher.read()
      parentPort.postMessage({ value: buffer }, [buffer])
    })
})

Reproduction here: https://github.com/timsuchanek/segfault-node-14

lldb backtrace

Process 40610 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x20)
    frame #0: 0x000000010007b095 node`node::Buffer::New(node::Environment*, char*, unsigned long, bool)::$_2::__invoke(void*, unsigned long, void*) + 21
node`node::Buffer::New(node::Environment*, char*, unsigned long, bool)::$_2::__invoke(void*, unsigned long, void*):
->  0x10007b095 <+21>: movq   0x20(%rcx), %rcx
    0x10007b099 <+25>: movq   %rax, %rdi
    0x10007b09c <+28>: popq   %rbp
    0x10007b09d <+29>: jmpq   *%rcx
Target 0: (node) stopped.

This works fine in Node 13 or lower and it seems, that this bug was introduced in Node 14.

[Jolg42]

I can also reproduce this when cloning the reproduction repository with Node 14.1.0

c3f5abe3e11d87d645b9e9fda1bad6a8d2f9e54f7e81478138ac134ba7ac7280
fish: 'node index.js' terminated by signal SIGSEGV (Address boundary error)

It works with Node 12 for me.

[juanarbol]

Thank you so much for the report and code recreation.

[juanarbol]

I'm not completely sure, but may be related with The new V8 ArrayBuffer API landed on Node v14.0.

[juanarbol]

cc @nodejs/buffer

[jasnell]

So this is related to this issue: #33240 and this PR #33252

Specifically, you're attempting to transfer an ArrayBuffer instance that cannot be transferred. We need to implement better protections around this throughout core but the fundamental idea is that you should never transfer a Buffer or TypedArray unless you know for absolute certain that it is safe to do so -- and that's generally only when you are creating it yourself. The fix in this particular case would be to create your own Uint8Array copy of the buffer before sending it...

In your worker... something like:

const fs = require('fs')
const crypto = require('crypto')
const { parentPort } = require('worker_threads')

parentPort.on('message', (message) => {
  const hasher = crypto.createHash('sha256')
  fs.createReadStream('example.txt')
    .pipe(hasher)
    .on('finish', () => {
      const { buffer } = hasher.read()
      const buf = new Uint8Array(buffer);  // Create a copy
      parentPort.postMessage({ value: buf }, [buf.buffer])
    })
})

Alternatively, it's not clear from this example why you are using hasher.read() at all. The example is definitely not a typical case. What I would imagine would be a better approach in general is something like...

const fs = require('fs')
const crypto = require('crypto')
const { parentPort } = require('worker_threads')
const { pipeline } = require('stream')

parentPort.on('message', (message) => {
  const input = fs.createReadStream('example.txt')
  const hasher = crypto.createHash('sha256')
  pipeline(input, hasher, (err) => {
    if (err) {
      // handle the error appropriately
      return;
    }
    // Pass a hex of the hash rather than the buffer
    parentPort.postMessage({ value: hasher.digest().toString('hex')});
  });
})

[jasnell]

Now... all that said... just as a more general point that is independent of the segfault issue that we really need to make sure we look at... given that read stream and the hash operations here are already async, you're not likely to see any real benefit from using a worker thread in this way (see https://github.com/jasnell/piscina/tree/master/examples/server for an example perf analysis). Specifically, the performance of the worker thread in this specific example will never be faster than just doing the same operations on the main thread.

[timsuchanek]

Thanks @jasnell for the insights! Sounds like a simplification can be done in the library hasha then, where that pattern is used.

[jasnell]

Ah, yes... I'll open an issue there

[jasnell]

I'm now unable to reproduce this issue on 14.x and 15.x. I believe the issue has been resolved tho I'm not sure exactly which commit fixed it. Closing, can reopen if it's still an issue