Skip to content

src: fix crash on empty, foreign or truncated --snapshot-blob files - #65955

Open
codebytere wants to merge 1 commit into
nodejs:mainfrom
codebytere:fix/embedder-snapshot-blob-validation
Open

src: fix crash on empty, foreign or truncated --snapshot-blob files#65955
codebytere wants to merge 1 commit into
nodejs:mainfrom
codebytere:fix/embedder-snapshot-blob-validation

Conversation

@codebytere

Copy link
Copy Markdown
Member

node --snapshot-blob <file> main.js aborts with an assertion when the file is empty, is not a Node.js snapshot, or has a zero-length startup blob, and reads past the end of the buffer when a snapshot is truncated, because BlobDeserializer trusted every length field in the blob; EmbedderSnapshotData::FromFile(), documented to return an empty pointer for an invalid snapshot, crashed the same way.

BlobDeserializer now bounds-checks each read and records the failure, SnapshotData::FromBlob() prints "not a Node.js snapshot blob" or "truncated" and returns false so the CLI exits with the usual snapshot error code, and ReadFileSync() accepts an empty file. Also adds the missing space in the "built with Node.js version" messages.

Tests: test/parallel/test-snapshot-invalid-blob.js covers empty, garbage, zero-length-blob and truncated files (all aborted or overread before); the snapshot suites pass.

Refs: #38905
Refs: #47933


Disclosure: the code, test and this description were written by Claude Code, directed and reviewed by @codebytere.

`node --snapshot-blob <file> main.js` aborted with an assertion when
the file was empty (`ReadFileSync()` insists on reading one item), was
not a Node.js snapshot (`CHECK_EQ(magic, kMagic)`) or had a zero-length
startup blob, and read past the end of the buffer when a snapshot was
truncated, because `BlobDeserializer` trusted every length field in the
blob. `EmbedderSnapshotData::FromFile()` is documented to return an
empty pointer for an invalid snapshot and crashed the same way.

Bounds-check each read in `BlobDeserializer` and record the failure,
have `SnapshotData::FromBlob()` print why and return false, and let
`ReadFileSync()` return an empty vector for an empty file. Also add the
missing space in the "built with Node.js version" messages.

Refs: nodejs#38905
Refs: nodejs#47933
Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup

@codebytere codebytere added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 10, 2026
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Sep 10, 2026
@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.96296% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.17%. Comparing base (b805fb5) to head (d9c0bc8).
⚠️ Report is 38 commits behind head on main.

Files with missing lines Patch % Lines
src/blob_serializer_deserializer-inl.h 50.00% 2 Missing and 3 partials ⚠️
src/node_snapshotable.cc 66.66% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #65955   +/-   ##
=======================================
  Coverage   90.17%   90.17%           
=======================================
  Files         771      771           
  Lines      265489   265510   +21     
  Branches    50471    50477    +6     
=======================================
+ Hits       239405   239432   +27     
+ Misses      17042    17010   -32     
- Partials     9042     9068   +26     
Files with missing lines Coverage Δ
src/blob_serializer_deserializer.h 100.00% <100.00%> (ø)
src/node_file_utils.cc 62.58% <100.00%> (+0.27%) ⬆️
src/blob_serializer_deserializer-inl.h 65.21% <50.00%> (-0.88%) ⬇️
src/node_snapshotable.cc 73.51% <66.66%> (+0.03%) ⬆️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@codebytere codebytere removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 10, 2026
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants