From 8641d94189398063b20d5e38549bfd8f023af2d6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 6 Jun 2020 16:27:30 +0200 Subject: [PATCH] worker,fs: make FileHandle transferable Allow passing `FileHandle` instances in the transfer list of a `.postMessage()` call. PR-URL: https://github.com/nodejs/node/pull/33772 Reviewed-By: Benjamin Gruenbaum --- doc/api/worker_threads.md | 12 +++- lib/internal/fs/promises.js | 28 +++++++- lib/internal/worker/js_transferable.js | 8 +++ src/node_file.cc | 34 ++++++++++ src/node_file.h | 22 +++++++ ...-transfer-fake-js-transferable-internal.js | 33 ++++++++++ ...sage-port-transfer-fake-js-transferable.js | 37 +++++++++++ ...worker-message-port-transfer-filehandle.js | 65 +++++++++++++++++++ 8 files changed, 235 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-worker-message-port-transfer-fake-js-transferable-internal.js create mode 100644 test/parallel/test-worker-message-port-transfer-fake-js-transferable.js create mode 100644 test/parallel/test-worker-message-port-transfer-filehandle.js diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md index d4fd2f5179a..9a893a40972 100644 --- a/doc/api/worker_threads.md +++ b/doc/api/worker_threads.md @@ -318,6 +318,10 @@ are part of the channel. ### `port.postMessage(value[, transferList])` * `value` {any} @@ -335,7 +339,8 @@ In particular, the significant differences to `JSON` are: * `value` may contain typed arrays, both using `ArrayBuffer`s and `SharedArrayBuffer`s. * `value` may contain [`WebAssembly.Module`][] instances. -* `value` may not contain native (C++-backed) objects other than `MessagePort`s. +* `value` may not contain native (C++-backed) objects other than `MessagePort`s + and [`FileHandle`][]s. ```js const { MessageChannel } = require('worker_threads'); @@ -349,7 +354,8 @@ circularData.foo = circularData; port2.postMessage(circularData); ``` -`transferList` may be a list of `ArrayBuffer` and `MessagePort` objects. +`transferList` may be a list of [`ArrayBuffer`][], [`MessagePort`][] and +[`FileHandle`][] objects. After transferring, they will not be usable on the sending side of the channel anymore (even if they are not contained in `value`). Unlike with [child processes][], transferring handles such as network sockets is currently @@ -816,6 +822,7 @@ active handle in the event system. If the worker is already `unref()`ed calling [`'close'` event]: #worker_threads_event_close [`'exit'` event]: #worker_threads_event_exit +[`ArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer [`AsyncResource`]: async_hooks.html#async_hooks_class_asyncresource [`Buffer`]: buffer.html [`Buffer.allocUnsafe()`]: buffer.html#buffer_class_method_buffer_allocunsafe_size @@ -823,6 +830,7 @@ active handle in the event system. If the worker is already `unref()`ed calling [`ERR_WORKER_NOT_RUNNING`]: errors.html#ERR_WORKER_NOT_RUNNING [`EventEmitter`]: events.html [`EventTarget`]: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget +[`FileHandle`]: fs.html#fs_class_filehandle [`MessagePort`]: #worker_threads_class_messageport [`SharedArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer [`Uint8Array`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 05d82991552..717b26e59fa 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -65,13 +65,17 @@ const { promisify } = require('internal/util'); const kHandle = Symbol('kHandle'); const kFd = Symbol('kFd'); const { kUsePromises } = binding; +const { + JSTransferable, kDeserialize, kTransfer, kTransferList +} = require('internal/worker/js_transferable'); const getDirectoryEntriesPromise = promisify(getDirents); -class FileHandle { +class FileHandle extends JSTransferable { constructor(filehandle) { + super(); this[kHandle] = filehandle; - this[kFd] = filehandle.fd; + this[kFd] = filehandle ? filehandle.fd : -1; } getAsyncId() { @@ -142,6 +146,26 @@ class FileHandle { this[kFd] = -1; return this[kHandle].close(); } + + [kTransfer]() { + const handle = this[kHandle]; + this[kFd] = -1; + this[kHandle] = null; + + return { + data: { handle }, + deserializeInfo: 'internal/fs/promises:FileHandle' + }; + } + + [kTransferList]() { + return [ this[kHandle] ]; + } + + [kDeserialize]({ handle }) { + this[kHandle] = handle; + this[kFd] = handle.fd; + } } function validateFileHandle(handle) { diff --git a/lib/internal/worker/js_transferable.js b/lib/internal/worker/js_transferable.js index 41f1e6ff72a..707fd03f2f6 100644 --- a/lib/internal/worker/js_transferable.js +++ b/lib/internal/worker/js_transferable.js @@ -1,4 +1,5 @@ 'use strict'; +const { Error } = primordials; const { messaging_deserialize_symbol, messaging_transfer_symbol, @@ -17,6 +18,13 @@ function setup() { setDeserializerCreateObjectFunction((deserializeInfo) => { const [ module, ctor ] = deserializeInfo.split(':'); const Ctor = require(module)[ctor]; + if (typeof Ctor !== 'function' || + !(Ctor.prototype instanceof JSTransferable)) { + // Not one of the official errors because one should not be able to get + // here without messing with Node.js internals. + // eslint-disable-next-line no-restricted-syntax + throw new Error(`Unknown deserialize spec ${deserializeInfo}`); + } return new Ctor(); }); } diff --git a/src/node_file.cc b/src/node_file.cc index c2ed56f43fb..9ed2d2960cf 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -190,6 +190,40 @@ void FileHandle::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("current_read", current_read_); } +FileHandle::TransferMode FileHandle::GetTransferMode() const { + return reading_ || closing_ || closed_ ? + TransferMode::kUntransferable : TransferMode::kTransferable; +} + +std::unique_ptr FileHandle::TransferForMessaging() { + CHECK_NE(GetTransferMode(), TransferMode::kUntransferable); + auto ret = std::make_unique(fd_); + closed_ = true; + return ret; +} + +FileHandle::TransferData::TransferData(int fd) : fd_(fd) {} + +FileHandle::TransferData::~TransferData() { + if (fd_ > 0) { + uv_fs_t close_req; + CHECK_EQ(0, uv_fs_close(nullptr, &close_req, fd_, nullptr)); + uv_fs_req_cleanup(&close_req); + } +} + +BaseObjectPtr FileHandle::TransferData::Deserialize( + Environment* env, + v8::Local context, + std::unique_ptr self) { + BindingData* bd = Environment::GetBindingData(context); + if (bd == nullptr) return {}; + + int fd = fd_; + fd_ = -1; + return BaseObjectPtr { FileHandle::New(bd, fd) }; +} + // Close the file descriptor if it hasn't already been closed. A process // warning will be emitted using a SetImmediate to avoid calling back to // JS during GC. If closing the fd fails at this point, a fatal exception diff --git a/src/node_file.h b/src/node_file.h index ad493bd944c..fd17fc99d52 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -5,6 +5,7 @@ #include "node.h" #include "aliased_buffer.h" +#include "node_messaging.h" #include "stream_base.h" #include @@ -273,7 +274,28 @@ class FileHandle final : public AsyncWrap, public StreamBase { FileHandle(const FileHandle&&) = delete; FileHandle& operator=(const FileHandle&&) = delete; + TransferMode GetTransferMode() const override; + std::unique_ptr TransferForMessaging() override; + private: + class TransferData : public worker::TransferData { + public: + explicit TransferData(int fd); + ~TransferData(); + + BaseObjectPtr Deserialize( + Environment* env, + v8::Local context, + std::unique_ptr self) override; + + SET_NO_MEMORY_INFO() + SET_MEMORY_INFO_NAME(FileHandleTransferData) + SET_SELF_SIZE(TransferData) + + private: + int fd_; + }; + FileHandle(BindingData* binding_data, v8::Local obj, int fd); // Synchronous close that emits a warning diff --git a/test/parallel/test-worker-message-port-transfer-fake-js-transferable-internal.js b/test/parallel/test-worker-message-port-transfer-fake-js-transferable-internal.js new file mode 100644 index 00000000000..10a50494338 --- /dev/null +++ b/test/parallel/test-worker-message-port-transfer-fake-js-transferable-internal.js @@ -0,0 +1,33 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const fs = require('fs').promises; +const { MessageChannel } = require('worker_threads'); +const { once } = require('events'); + +// Test that overriding the internal kTransfer method of a JSTransferable does +// not enable loading arbitrary code from internal Node.js core modules. + +(async function() { + const fh = await fs.open(__filename); + assert.strictEqual(fh.constructor.name, 'FileHandle'); + + const kTransfer = Object.getOwnPropertySymbols(Object.getPrototypeOf(fh)) + .filter((symbol) => symbol.description === 'messaging_transfer_symbol')[0]; + assert.strictEqual(typeof kTransfer, 'symbol'); + fh[kTransfer] = () => { + return { + data: '✨', + deserializeInfo: 'net:Socket' + }; + }; + + const { port1, port2 } = new MessageChannel(); + port1.postMessage(fh, [ fh ]); + port2.on('message', common.mustNotCall()); + + const [ exception ] = await once(process, 'uncaughtException'); + + assert.strictEqual(exception.message, 'Unknown deserialize spec net:Socket'); + port2.close(); +})().then(common.mustCall()); diff --git a/test/parallel/test-worker-message-port-transfer-fake-js-transferable.js b/test/parallel/test-worker-message-port-transfer-fake-js-transferable.js new file mode 100644 index 00000000000..924d850a5d1 --- /dev/null +++ b/test/parallel/test-worker-message-port-transfer-fake-js-transferable.js @@ -0,0 +1,37 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const fs = require('fs').promises; +const { MessageChannel } = require('worker_threads'); +const { once } = require('events'); + +// Test that overriding the internal kTransfer method of a JSTransferable does +// not enable loading arbitrary code from the disk. + +module.exports = { + NotARealClass: common.mustNotCall() +}; + +(async function() { + const fh = await fs.open(__filename); + assert.strictEqual(fh.constructor.name, 'FileHandle'); + + const kTransfer = Object.getOwnPropertySymbols(Object.getPrototypeOf(fh)) + .filter((symbol) => symbol.description === 'messaging_transfer_symbol')[0]; + assert.strictEqual(typeof kTransfer, 'symbol'); + fh[kTransfer] = () => { + return { + data: '✨', + deserializeInfo: `${__filename}:NotARealClass` + }; + }; + + const { port1, port2 } = new MessageChannel(); + port1.postMessage(fh, [ fh ]); + port2.on('message', common.mustNotCall()); + + const [ exception ] = await once(process, 'uncaughtException'); + + assert.match(exception.message, /Missing internal module/); + port2.close(); +})().then(common.mustCall()); diff --git a/test/parallel/test-worker-message-port-transfer-filehandle.js b/test/parallel/test-worker-message-port-transfer-filehandle.js new file mode 100644 index 00000000000..3765fca865e --- /dev/null +++ b/test/parallel/test-worker-message-port-transfer-filehandle.js @@ -0,0 +1,65 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const fs = require('fs').promises; +const vm = require('vm'); +const { MessageChannel, moveMessagePortToContext } = require('worker_threads'); +const { once } = require('events'); + +(async function() { + const fh = await fs.open(__filename); + + const { port1, port2 } = new MessageChannel(); + + assert.throws(() => { + port1.postMessage(fh); + }, { + // See the TODO about error code in node_messaging.cc. + code: 'ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST' + }); + + // Check that transferring FileHandle instances works. + assert.notStrictEqual(fh.fd, -1); + port1.postMessage(fh, [ fh ]); + assert.strictEqual(fh.fd, -1); + + const [ fh2 ] = await once(port2, 'message'); + assert.strictEqual(Object.getPrototypeOf(fh2), Object.getPrototypeOf(fh)); + + assert.deepStrictEqual(await fh2.readFile(), await fs.readFile(__filename)); + await fh2.close(); + + assert.rejects(() => fh.readFile(), { code: 'EBADF' }); +})().then(common.mustCall()); + +(async function() { + // Check that there is no crash if the message is never read. + const fh = await fs.open(__filename); + + const { port1 } = new MessageChannel(); + + assert.notStrictEqual(fh.fd, -1); + port1.postMessage(fh, [ fh ]); + assert.strictEqual(fh.fd, -1); +})().then(common.mustCall()); + +(async function() { + // Check that in the case of a context mismatch the message is discarded. + const fh = await fs.open(__filename); + + const { port1, port2 } = new MessageChannel(); + + const ctx = vm.createContext(); + const port2moved = moveMessagePortToContext(port2, ctx); + port2moved.onmessage = common.mustCall((msgEvent) => { + assert.strictEqual(msgEvent.data, 'second message'); + port1.close(); + }); + port2moved.start(); + + assert.notStrictEqual(fh.fd, -1); + port1.postMessage(fh, [ fh ]); + assert.strictEqual(fh.fd, -1); + + port1.postMessage('second message'); +})().then(common.mustCall());