src: mark generated `snapshot_data` as `const` by addaleax · Pull Request #45786 · nodejs/node

addaleax · 2022-12-08T13:43:52Z

This renders the mutex protecting it unnecessary, since mutexes only need to protect concurrent accesses to mutable data.

nodejs-github-bot · 2022-12-08T13:43:57Z

Review requested:

@nodejs/startup

github-actions · 2022-12-08T13:44:15Z

Fast-track has been requested by @addaleax. Please 👍 to approve.

nodejs-github-bot · 2022-12-08T13:51:51Z

CI: https://ci.nodejs.org/job/node-test-pull-request/48345/

nodejs-github-bot · 2022-12-08T15:12:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/48349/

joyeecheung

We actually have a const cast here

joyeecheung · 2022-12-08T15:30:55Z

src/node_snapshotable.cc

  context->AllowCodeGenerationFromStrings(true);
 }

-Mutex SnapshotBuilder::snapshot_data_mutex_;


We actually have a const_cast at L1064, though I think that V8 doesn't actually need params->snapshot_blob to be non-const. Can you modify the v8::CreateParams::snapshot_blob to a const pointer too and see if it works? If so we should send a patch to the upstream first to get rid of the const cast, so that they don't start to actually mutate the blob without us knowing about it.

Uh-oh, it appears v8 is const-casting too.. 😅

node/deps/v8/src/snapshot/snapshot-data.h

Line 77 in 50930a0

: SerializedData(const_cast<byte*>(snapshot.begin()), snapshot.length()) {

Hm yeah… our const_cast is technically invalid then as well, right? But yeah, agreed that the right thing to do here is to send a patch upstream. I’ll try to put one together and link it here.

I think what you’re bringing up is actually an argument to make this change either way, though. V8 really can’t mutate the blob, since it needs to be re-usable across Isolates (both concurrently and sequentially). If V8 were to mutate the blob, we would need to learn about that; and an easy way to do that is to make the actual data const so that attempting to mutate it crashes.

Actually, V8’s StartupData is just a const char* data + int raw_size. That can’t really be mutated by V8 anyway.

Yeah, it looks like V8 isn't actually mutating the data, just reusing the data structure that is also used for serialization. Perhaps some additional field in SerializedData that can be used to DCHECK mutation in non-const methods would be enough.

If V8 were to mutate the blob, we would need to learn about that; and an easy way to do that is to make the actual data const so that attempting to mutate it crashes.

Isn't mutating const-casted data undefined behavior, so it's not certain that it would crash (or it would crash reliably)? And that's my main concern because we might be seeing crashes that can't be easily link to the mutation, depending on how the mutation happens..it'd probably be safer to have some DCHECK against the mutation, either in V8 or we DCHECK a verification of the snapshot checksum when we initialize a new isolate with the snapshot data.

V8 really can’t mutate the blob, since it needs to be re-usable across Isolates (both concurrently and sequentially).

I think you are talking about the default snapshot, not the one passed into IsolateParams? V8 also has a mutex to guard its default snapshot read from disk:

node/deps/v8/src/snapshot/snapshot-external.cc

Line 57 in 6bd756d

base::MutexGuard lock_guard(external_startup_data_mutex.Pointer());

(not entirely sure if that's strictly necessary for V8 either, but I think that's why we also have a mutex in our own code)

(On a second thought, why is the mutex used in an accessor (both in our code and in V8)? It probably should've been somewhere surrounding Isolate::Initialize() instead to serve that purpose..

Oh I see, the V8 mutex is guarding against refreshing the snapshot while it's being read for isolate initialization, which isn't a thing for us because we don't refresh our embedded snapshot blob in any way. The one we read from disk is in a separate location. So we really don't need this mutex anyway (and if we really want to avoid a race from mutation from V8, we should have a mutex surrounding Isolate::Initialize() instead). But with the V8 CL if V8 mutates the const-pointed snapshot in Isolate::Initialize(), it's their bug, not ours, so we should just do this anyway.

Isn't mutating const-casted data undefined behavior, so it's not certain that it would crash (or it would crash reliably)?

From a language perspective, yeah, it's UB. I was honestly expecting that marking this as const would put it in the .rodata section of the binary; looking at the compiled result, it seems like it's a bit too complex for that.

In any case, the V8 CL is merged now, if you'd like me to cherry-pick the IsolateParams change and remove the const_cast I'm happy to also do that.

DCHECK a verification of the snapshot checksum

Just to avoid disambiguity, this PR does not affect the const-ness of the actual snapshot data; that's already a const char* anyway (which also does actually end up in .rodata). If V8 does const_cast on that, then yeah, as you point out, it's on them to make sure they do it properly.

joyeecheung

Blocking until we get rid of the const cast

joyeecheung · 2022-12-08T21:17:10Z

src/node_snapshotable.cc

  context->AllowCodeGenerationFromStrings(true);
 }

-Mutex SnapshotBuilder::snapshot_data_mutex_;


Oh I see, the V8 mutex is guarding against refreshing the snapshot while it's being read for isolate initialization, which isn't a thing for us because we don't refresh our embedded snapshot blob in any way. The one we read from disk is in a separate location. So we really don't need this mutex anyway (and if we really want to avoid a race from mutation from V8, we should have a mutex surrounding Isolate::Initialize() instead). But with the V8 CL if V8 mutates the const-pointed snapshot in Isolate::Initialize(), it's their bug, not ours, so we should just do this anyway.

nodejs-github-bot · 2022-12-10T17:16:25Z

Landed in 94d23f5

This renders the mutex protecting it unnecessary, since mutexes only need to protect concurrent accesses to mutable data. PR-URL: nodejs#45786 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

This renders the mutex protecting it unnecessary, since mutexes only need to protect concurrent accesses to mutable data. PR-URL: #45786 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

src: mark generated snapshot_data as const

2667f49

This renders the mutex protecting it unnecessary, since mutexes only need to protect concurrent accesses to mutable data.

addaleax requested a review from joyeecheung December 8, 2022 13:43

addaleax added fast-track PRs that do not need to wait for 48 hours to land. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 8, 2022

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Dec 8, 2022

cjihrig approved these changes Dec 8, 2022

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 8, 2022

anonrig approved these changes Dec 8, 2022

View reviewed changes

anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 8, 2022

joyeecheung reviewed Dec 8, 2022

View reviewed changes

joyeecheung removed the fast-track PRs that do not need to wait for 48 hours to land. label Dec 8, 2022

joyeecheung requested changes Dec 8, 2022

View reviewed changes

joyeecheung approved these changes Dec 8, 2022

View reviewed changes

JungMinu approved these changes Dec 9, 2022

View reviewed changes

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2022

nodejs-github-bot merged commit 94d23f5 into nodejs:main Dec 10, 2022

addaleax deleted the snapshot-data-const-mutex branch December 10, 2022 18:05

targos mentioned this pull request Dec 12, 2022

v19.3.0 release proposal #45831

Merged

danielleadams mentioned this pull request Dec 30, 2022

v18.13.0 release proposal #46025

Merged

Uh oh!

Conversation

addaleax commented Dec 8, 2022

Uh oh!

nodejs-github-bot commented Dec 8, 2022

Uh oh!

github-actions bot commented Dec 8, 2022

Uh oh!

nodejs-github-bot commented Dec 8, 2022

Uh oh!

nodejs-github-bot commented Dec 8, 2022

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

joyeecheung Dec 8, 2022 •

edited

Loading

addaleax Dec 8, 2022 •

edited

Loading

joyeecheung Dec 8, 2022 •

edited

Loading

joyeecheung Dec 8, 2022 •

edited

Loading

joyeecheung Dec 8, 2022 •

edited

Loading

joyeecheung Dec 8, 2022 •

edited

Loading