Skip to content

Commit a3ababc

Browse files
codebytereaduh95
codebytere
authored and
aduh95
committed
src: fix null pointer call when running without a startup snapshot
Without a startup snapshot (`--no-node-snapshot`, a `--without-node-snapshot` build, or an embedder Environment that was bootstrapped from scratch) starting a Worker made a member call through a null `SnapshotData*`, and so did `NodeMainInstance` while setting itself up. It only worked because the function called does not touch `this`; UBSan reports it for every such Worker. The call existed because `IsolateData::CreateIsolateData()` took an `EmbedderSnapshotData*` and unwrapped it straight away, so the two internal callers wrapped their possibly-null `SnapshotData*` with `AsEmbedderWrapper()` only for it to be unwrapped again. Let the internal function take the `SnapshotData*` itself, unwrap in the public `CreateIsolateData()` only, and drop `AsEmbedderWrapper()`, which has no other users. Refs: #47731 Signed-off-by: Shelley Vohr <[email protected]> PR-URL: #65820 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
1 parent 843a277 commit a3ababc

7 files changed

Lines changed: 31 additions & 22 deletions

File tree

‎src/api/environment.cc‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -393,7 +393,11 @@ IsolateData* CreateIsolateData(
393393
ArrayBufferAllocator* allocator,
394394
const EmbedderSnapshotData* embedder_snapshot_data) {
395395
return IsolateData::CreateIsolateData(
396-
isolate, loop, platform, allocator, embedder_snapshot_data);
396+
isolate,
397+
loop,
398+
platform,
399+
allocator,
400+
SnapshotData::FromEmbedderWrapper(embedder_snapshot_data));
397401
}
398402

399403
void FreeIsolateData(IsolateData* isolate_data) {

‎src/env.cc‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -589,10 +589,8 @@ IsolateData* IsolateData::CreateIsolateData(
589589
uv_loop_t* loop,
590590
MultiIsolatePlatform* platform,
591591
ArrayBufferAllocator* allocator,
592-
const EmbedderSnapshotData* embedder_snapshot_data,
592+
const SnapshotData* snapshot_data,
593593
std::shared_ptr<PerIsolateOptions> options) {
594-
const SnapshotData* snapshot_data =
595-
SnapshotData::FromEmbedderWrapper(embedder_snapshot_data);
596594
if (options == nullptr) {
597595
options = per_process::cli_options->per_isolate->Clone();
598596
}

‎src/env.h‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
142142
uv_loop_t* event_loop,
143143
MultiIsolatePlatform* platform = nullptr,
144144
ArrayBufferAllocator* node_allocator = nullptr,
145-
const EmbedderSnapshotData* embedder_snapshot_data = nullptr,
145+
const SnapshotData* snapshot_data = nullptr,
146146
std::shared_ptr<PerIsolateOptions> options = nullptr);
147147
~IsolateData();
148148

@@ -646,7 +646,6 @@ struct SnapshotData {
646646
static bool FromBlob(SnapshotData* out, std::string_view in);
647647
static const SnapshotData* FromEmbedderWrapper(
648648
const EmbedderSnapshotData* data);
649-
EmbedderSnapshotData::Pointer AsEmbedderWrapper() const;
650649

651650
~SnapshotData();
652651
};

‎src/node_main_instance.cc‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -51,11 +51,11 @@ NodeMainInstance::NodeMainInstance(const SnapshotData* snapshot_data,
5151

5252
// If the indexes are not nullptr, we are not deserializing
5353
isolate_data_.reset(
54-
CreateIsolateData(isolate_,
55-
event_loop,
56-
platform,
57-
array_buffer_allocator_.get(),
58-
snapshot_data->AsEmbedderWrapper().get()));
54+
IsolateData::CreateIsolateData(isolate_,
55+
event_loop,
56+
platform,
57+
array_buffer_allocator_.get(),
58+
snapshot_data));
5959

6060
isolate_data_->max_young_gen_size =
6161
isolate_params_->constraints.max_young_generation_size_in_bytes();

‎src/node_snapshotable.cc‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -630,10 +630,6 @@ const SnapshotData* SnapshotData::FromEmbedderWrapper(
630630
return data != nullptr ? data->impl_ : nullptr;
631631
}
632632

633-
EmbedderSnapshotData::Pointer SnapshotData::AsEmbedderWrapper() const {
634-
return EmbedderSnapshotData::Pointer{new EmbedderSnapshotData(this, false)};
635-
}
636-
637633
bool SnapshotData::FromFile(SnapshotData* out, FILE* in) {
638634
return FromBlob(out, ReadFileSync(in));
639635
}

‎src/node_worker.cc‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -205,13 +205,13 @@ class WorkerThreadData {
205205
isolate->SetStackLimit(w->stack_base_);
206206

207207
HandleScope handle_scope(isolate);
208-
isolate_data_.reset(IsolateData::CreateIsolateData(
209-
isolate,
210-
&loop_,
211-
w_->platform_,
212-
allocator.get(),
213-
w->snapshot_data()->AsEmbedderWrapper().get(),
214-
std::move(w_->per_isolate_opts_)));
208+
isolate_data_.reset(
209+
IsolateData::CreateIsolateData(isolate,
210+
&loop_,
211+
w_->platform_,
212+
allocator.get(),
213+
w->snapshot_data(),
214+
std::move(w_->per_isolate_opts_)));
215215
CHECK(isolate_data_);
216216
CHECK(!isolate_data_->is_building_snapshot());
217217
isolate_data_->set_worker_context(w_);

‎test/cctest/test_environment.cc‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,18 @@ TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) {
353353
EXPECT_TRUE(called_cb_2);
354354
}
355355

356+
TEST_F(EnvironmentTest, WorkerInEnvironmentWithoutSnapshot) {
357+
const v8::HandleScope handle_scope(isolate_);
358+
const Argv argv;
359+
Env env{handle_scope, argv};
360+
CHECK_NULL(isolate_data_->snapshot_data());
361+
node::LoadEnvironment(*env,
362+
"const { Worker } = require('worker_threads');"
363+
"new Worker('process.exit(0)', { eval: true });")
364+
.ToLocalChecked();
365+
EXPECT_EQ(node::SpinEventLoop(*env).FromJust(), 0);
366+
}
367+
356368
TEST_F(EnvironmentTest, NoEnvironmentSanity) {
357369
const v8::HandleScope handle_scope(isolate_);
358370
v8::Local<v8::Context> context = v8::Context::New(isolate_);

0 commit comments

Comments
 (0)