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

Cannot construct new Buffer in worker thread after transferring buffer out #32752

Closed
daguej opened this issue Apr 10, 2020 · 5 comments
Closed
Labels
buffer Issues and PRs related to the buffer subsystem.

Comments

@daguej
Copy link
Contributor

daguej commented Apr 10, 2020

After transferring a Buffer instance to a different thread, it becomes impossible to construct new Buffers in the current thread.

  • Version: v12.16.2
  • Platform: win10 x64
  • Subsystem: buffer, worker_threads

What steps will reproduce the bug?

var worker_threads = require('worker_threads');

if (worker_threads.isMainThread) {
	var worker = new worker_threads.Worker(__filename);

	worker.on('message', function(msg) {
		console.log(msg);
	});
	
} else {
	
	try {
		sendBuf(Buffer.from('hello world'));
		sendBuf(Buffer.from('hello world'));
	} catch (ex) {
		console.error(ex);
	}


}

function sendBuf(buf) {
	worker_threads.parentPort.postMessage({
		buf: buf
	}, [ buf.buffer ]);
}

What is the expected behavior?

It should not crash node

What do you see instead?

{
  buf: Uint8Array(11) [
    104, 101, 108, 108,
    111,  32, 119, 111,
    114, 108, 100
  ]
}
TypeError: Cannot perform Construct on a neutered ArrayBuffer
    at new Uint8Array (<anonymous>)
    at new FastBuffer (internal/buffer.js:945:1)
    at fromStringFast (buffer.js:427:11)
    at fromString (buffer.js:452:10)
    at Function.from (buffer.js:302:12)
    at Object.<anonymous> (/worker-test.js:14:18)
    at Module._compile (internal/modules/cjs/loader.js:1156:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1176:10)
    at Module.load (internal/modules/cjs/loader.js:1000:32)
    at Function.Module._load (internal/modules/cjs/loader.js:899:14)

It looks like after the first Buffer is transferred out, the allocPool contains a now-neutered ArrayBuffer.

If I call createPool() before

let b = new FastBuffer(allocPool, poolOffset, length);

then the operation succeeds.

@addaleax addaleax added the buffer Issues and PRs related to the buffer subsystem. label Apr 10, 2020
@addaleax
Copy link
Member

I guess it’s fair to expect this to not stop Node from creating new buffers, but if you transfer a pooled Buffer’s ArrayBuffer, bad things are bound to happen either way.

@daguej
Copy link
Contributor Author

daguej commented Apr 10, 2020

Perhaps I'm not understanding the transfer API correctly -- is there something wrong with my example code?

  1. I allocate a new Buffer and put some data in it.
  2. Using postMessage, the buffer is transferred to a different thread.
  3. I try to allocate another new (unrelated) Buffer, but now the operation fails.

From the perspective of user code, I think that usage should be fine. If there's pooling going on, isn't that an internal implementation detail that user code shouldn't have to worry about?

If there's something different my example code should be doing, what is it?

Either way, it should definitely not be possible to get in to a state where it's impossible to create new buffers.

@addaleax
Copy link
Member

addaleax commented Apr 10, 2020

@daguej I agree that this situation is unfortunate.

2. Using postMessage, the buffer is transferred to a different thread.

That’s what it looks like to you, but we do warn in the documentation that buf and buf.buffer are not the same thing and do not necessarily cover the same memory range. When you transfer buf.buffer, you will end up transferring a lot more then just the contents of buf itself.

I think what we could do here is to mark the pool for Buffers as untransferable. That way, this footgun is removed, and performance-wise, it should be fine because pooled Buffers are small-ish by default anyway.

addaleax added a commit to addaleax/node that referenced this issue Apr 10, 2020
This removes a footgun in which users could attempt to transfer the
pooled ArrayBuffer underlying a regular `Buffer`, which would lead to
all `Buffer`s that share the same pool being rendered unusable as well,
and potentially break creation of new pooled `Buffer`s.

This disables this kind of transfer.

Refs: nodejs#32752
@daguej
Copy link
Contributor Author

daguej commented Apr 10, 2020

OK, I see. I had read that documentation but unfortunately it didn't really click that it meant small Buffers get created using a shared backing allocation, and attempting to transfer that shared backing would cause problems.

Thanks for the quick response and patch.

Meanwhile, to guard against doing this in unpatched versions of node, what's the best way to detect that a Buffer has a shared backing ArrayBuffer so I don't attempt to transfer it?

buf.length != buf.buffer.byteLength? buf.length <= Buffer.poolSize?

@addaleax
Copy link
Member

buf.length != buf.buffer.byteLength?

Yup, that seems just right and can handle other kinds of pooling situations as well, not just Node’s pool for Buffer 👍

addaleax added a commit that referenced this issue Apr 13, 2020
This removes a footgun in which users could attempt to transfer the
pooled ArrayBuffer underlying a regular `Buffer`, which would lead to
all `Buffer`s that share the same pool being rendered unusable as well,
and potentially break creation of new pooled `Buffer`s.

This disables this kind of transfer.

Refs: #32752

PR-URL: #32759
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this issue Apr 14, 2020
This removes a footgun in which users could attempt to transfer the
pooled ArrayBuffer underlying a regular `Buffer`, which would lead to
all `Buffer`s that share the same pool being rendered unusable as well,
and potentially break creation of new pooled `Buffer`s.

This disables this kind of transfer.

Refs: #32752

PR-URL: #32759
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this issue Apr 25, 2020
This removes a footgun in which users could attempt to transfer the
pooled ArrayBuffer underlying a regular `Buffer`, which would lead to
all `Buffer`s that share the same pool being rendered unusable as well,
and potentially break creation of new pooled `Buffer`s.

This disables this kind of transfer.

Refs: nodejs#32752

PR-URL: nodejs#32759
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this issue Apr 28, 2020
This removes a footgun in which users could attempt to transfer the
pooled ArrayBuffer underlying a regular `Buffer`, which would lead to
all `Buffer`s that share the same pool being rendered unusable as well,
and potentially break creation of new pooled `Buffer`s.

This disables this kind of transfer.

Refs: #32752

PR-URL: #32759
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Apr 28, 2020
This removes a footgun in which users could attempt to transfer the
pooled ArrayBuffer underlying a regular `Buffer`, which would lead to
all `Buffer`s that share the same pool being rendered unusable as well,
and potentially break creation of new pooled `Buffer`s.

This disables this kind of transfer.

Refs: #32752

PR-URL: #32759
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants