Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit d4e397a

Browse filesBrowse files
addaleaxtargos
addaleax
authored and
targos
committed
worker: fix crash when SharedArrayBuffer outlives creating thread
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>
1 parent c5edeb9 commit d4e397a
Copy full SHA for d4e397a

File tree

Expand file treeCollapse file tree

6 files changed

+83
-9
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+83
-9
lines changed
Open diff view settings
Collapse file

‎src/node_worker.cc‎

Copy file name to clipboardExpand all lines: src/node_worker.cc
+8-5Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ Worker::Worker(Environment* env,
5959
per_isolate_opts_(per_isolate_opts),
6060
exec_argv_(exec_argv),
6161
platform_(env->isolate_data()->platform()),
62+
array_buffer_allocator_(ArrayBufferAllocator::Create()),
6263
start_profiler_idle_notifier_(env->profiler_idle_notifier_started()),
6364
thread_id_(Environment::AllocateThreadId()),
6465
env_vars_(env->env_vars()) {
@@ -102,17 +103,20 @@ bool Worker::is_stopped() const {
102103
return stopped_;
103104
}
104105

106+
std::shared_ptr<ArrayBufferAllocator> Worker::array_buffer_allocator() {
107+
return array_buffer_allocator_;
108+
}
109+
105110
// This class contains data that is only relevant to the child thread itself,
106111
// and only while it is running.
107112
// (Eventually, the Environment instance should probably also be moved here.)
108113
class WorkerThreadData {
109114
public:
110115
explicit WorkerThreadData(Worker* w)
111-
: w_(w),
112-
array_buffer_allocator_(ArrayBufferAllocator::Create()) {
116+
: w_(w) {
113117
CHECK_EQ(uv_loop_init(&loop_), 0);
114118

115-
Isolate* isolate = NewIsolate(array_buffer_allocator_.get(), &loop_);
119+
Isolate* isolate = NewIsolate(w->array_buffer_allocator_.get(), &loop_);
116120
CHECK_NOT_NULL(isolate);
117121

118122
{
@@ -124,7 +128,7 @@ class WorkerThreadData {
124128
isolate_data_.reset(CreateIsolateData(isolate,
125129
&loop_,
126130
w_->platform_,
127-
array_buffer_allocator_.get()));
131+
w->array_buffer_allocator_.get()));
128132
CHECK(isolate_data_);
129133
if (w_->per_isolate_opts_)
130134
isolate_data_->set_options(std::move(w_->per_isolate_opts_));
@@ -166,7 +170,6 @@ class WorkerThreadData {
166170
private:
167171
Worker* const w_;
168172
uv_loop_t loop_;
169-
std::unique_ptr<ArrayBufferAllocator> array_buffer_allocator_;
170173
DeleteFnPtr<IsolateData, FreeIsolateData> isolate_data_;
171174

172175
friend class Worker;
Collapse file

‎src/node_worker.h‎

Copy file name to clipboardExpand all lines: src/node_worker.h
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ class Worker : public AsyncWrap {
4141
SET_SELF_SIZE(Worker)
4242

4343
bool is_stopped() const;
44+
std::shared_ptr<ArrayBufferAllocator> array_buffer_allocator();
4445

4546
static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
4647
static void CloneParentEnvVars(
@@ -59,6 +60,7 @@ class Worker : public AsyncWrap {
5960
std::vector<std::string> argv_;
6061

6162
MultiIsolatePlatform* platform_;
63+
std::shared_ptr<ArrayBufferAllocator> array_buffer_allocator_;
6264
v8::Isolate* isolate_ = nullptr;
6365
bool start_profiler_idle_notifier_;
6466
uv_thread_t tid_;
Collapse file

‎src/sharedarraybuffer_metadata.cc‎

Copy file name to clipboardExpand all lines: src/sharedarraybuffer_metadata.cc
+14-3Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
#include "sharedarraybuffer_metadata.h"
22

33
#include "base_object-inl.h"
4+
#include "memory_tracker-inl.h"
45
#include "node_errors.h"
6+
#include "node_worker.h"
57
#include "util-inl.h"
68

79
#include <utility>
@@ -91,8 +93,16 @@ SharedArrayBufferMetadata::ForSharedArrayBuffer(
9193
return nullptr;
9294
}
9395

96+
// If the SharedArrayBuffer is coming from a Worker, we need to make sure
97+
// that the corresponding ArrayBuffer::Allocator lives at least as long as
98+
// the SharedArrayBuffer itself.
99+
worker::Worker* w = env->worker_context();
100+
std::shared_ptr<v8::ArrayBuffer::Allocator> allocator =
101+
w != nullptr ? w->array_buffer_allocator() : nullptr;
102+
94103
SharedArrayBuffer::Contents contents = source->Externalize();
95-
SharedArrayBufferMetadataReference r(new SharedArrayBufferMetadata(contents));
104+
SharedArrayBufferMetadataReference r(
105+
new SharedArrayBufferMetadata(contents, allocator));
96106
if (r->AssignToSharedArrayBuffer(env, context, source).IsNothing())
97107
return nullptr;
98108
return r;
@@ -114,8 +124,9 @@ Maybe<bool> SharedArrayBufferMetadata::AssignToSharedArrayBuffer(
114124
}
115125

116126
SharedArrayBufferMetadata::SharedArrayBufferMetadata(
117-
const SharedArrayBuffer::Contents& contents)
118-
: contents_(contents) { }
127+
const SharedArrayBuffer::Contents& contents,
128+
std::shared_ptr<v8::ArrayBuffer::Allocator> allocator)
129+
: contents_(contents), allocator_(allocator) { }
119130

120131
SharedArrayBufferMetadata::~SharedArrayBufferMetadata() {
121132
contents_.Deleter()(contents_.Data(),
Collapse file

‎src/sharedarraybuffer_metadata.h‎

Copy file name to clipboardExpand all lines: src/sharedarraybuffer_metadata.h
+4-1Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,9 @@ class SharedArrayBufferMetadata
4646
SharedArrayBufferMetadata(const SharedArrayBufferMetadata&) = delete;
4747

4848
private:
49-
explicit SharedArrayBufferMetadata(const v8::SharedArrayBuffer::Contents&);
49+
SharedArrayBufferMetadata(
50+
const v8::SharedArrayBuffer::Contents&,
51+
std::shared_ptr<v8::ArrayBuffer::Allocator>);
5052

5153
// Attach a lifetime tracker object with a reference count to `target`.
5254
v8::Maybe<bool> AssignToSharedArrayBuffer(
@@ -55,6 +57,7 @@ class SharedArrayBufferMetadata
5557
v8::Local<v8::SharedArrayBuffer> target);
5658

5759
v8::SharedArrayBuffer::Contents contents_;
60+
std::shared_ptr<v8::ArrayBuffer::Allocator> allocator_;
5861
};
5962

6063
} // namespace worker
Collapse file

‎test/parallel/test-worker-arraybuffer-zerofill.js‎

Copy file name to clipboard
+33Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
'use strict';
2+
require('../common');
3+
const assert = require('assert');
4+
const { Worker } = require('worker_threads');
5+
6+
// Make sure that allocating uninitialized ArrayBuffers in one thread does not
7+
// affect the zero-initialization in other threads.
8+
9+
const w = new Worker(`
10+
const { parentPort } = require('worker_threads');
11+
12+
function post() {
13+
const uint32array = new Uint32Array(64);
14+
parentPort.postMessage(uint32array.reduce((a, b) => a + b));
15+
}
16+
17+
setInterval(post, 0);
18+
`, { eval: true });
19+
20+
function allocBuffers() {
21+
Buffer.allocUnsafe(32 * 1024 * 1024);
22+
}
23+
24+
const interval = setInterval(allocBuffers, 0);
25+
26+
let messages = 0;
27+
w.on('message', (sum) => {
28+
assert.strictEqual(sum, 0);
29+
if (messages++ === 100) {
30+
clearInterval(interval);
31+
w.terminate();
32+
}
33+
});
Collapse file

‎test/parallel/test-worker-sharedarraybuffer-from-worker-thread.js‎

Copy file name to clipboard
+22Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
const { Worker } = require('worker_threads');
5+
6+
// Regression test for https://github.com/nodejs/node/issues/28777
7+
// Make sure that SharedArrayBuffers created in Worker threads are accessible
8+
// after the creating thread ended.
9+
10+
const w = new Worker(`
11+
const { parentPort } = require('worker_threads');
12+
const sharedArrayBuffer = new SharedArrayBuffer(4);
13+
parentPort.postMessage(sharedArrayBuffer);
14+
`, { eval: true });
15+
16+
let sharedArrayBuffer;
17+
w.once('message', common.mustCall((message) => sharedArrayBuffer = message));
18+
w.once('exit', common.mustCall(() => {
19+
const uint8array = new Uint8Array(sharedArrayBuffer);
20+
uint8array[0] = 42;
21+
assert.deepStrictEqual(uint8array, new Uint8Array([42, 0, 0, 0]));
22+
}));

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.