src: fix crash on empty, foreign or truncated --snapshot-blob files · nodejs/node@a05023f · GitHub
Skip to content

Commit a05023f

Browse files
codebytereaduh95
authored andcommitted
src: fix crash on empty, foreign or truncated --snapshot-blob files
`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: #38905 Refs: #47933 Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com> PR-URL: #65955 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent f2aa27d commit a05023f

5 files changed

Lines changed: 88 additions & 9 deletions

File tree

‎src/blob_serializer_deserializer-inl.h‎

Lines changed: 14 additions & 0 deletions

‎src/blob_serializer_deserializer.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,9 @@ class BlobDeserializer : public BlobSerializerDeserializer {
4545

4646
size_t read_total = 0;
4747
std::string_view sink;
48+
// Cleared when a read would go past the end of `sink`; that read and all
49+
// later ones yield zeroes and empty views, so callers can check at the end.
50+
bool ok = true;
4851

4952
Impl* impl() { return static_cast<Impl*>(this); }
5053
const Impl* impl() const { return static_cast<const Impl*>(this); }

‎src/node_file_utils.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,7 @@ std::vector<char> ReadFileSync(FILE* fp) {
240240
CHECK_EQ(err, 0);
241241

242242
std::vector<char> contents(size);
243+
if (size == 0) return contents;
243244
size_t num_read = fread(contents.data(), size, 1, fp);
244245
CHECK_EQ(num_read, 1);
245246
return contents;

‎src/node_snapshotable.cc‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -186,12 +186,16 @@ v8::StartupData SnapshotDeserializer::Read() {
186186
int raw_size = ReadArithmetic<int>();
187187
Debug("size=%d\n", raw_size);
188188

189-
CHECK_GT(raw_size, 0); // There should be no startup data of size 0.
189+
if (raw_size <= 0 ||
190+
static_cast<size_t>(raw_size) > sink.size() - read_total) {
191+
ok = false;
192+
return v8::StartupData{nullptr, 0};
193+
}
190194
// The data pointer of v8::StartupData would be deleted so it must be new'ed.
191-
std::unique_ptr<char> buf = std::unique_ptr<char>(new char[raw_size]);
192-
ReadArithmetic<char>(buf.get(), raw_size);
195+
char* buf = new char[raw_size];
196+
ReadArithmetic<char>(buf, raw_size);
193197

194-
return v8::StartupData{buf.release(), raw_size};
198+
return v8::StartupData{buf, raw_size};
195199
}
196200

197201
template <>
@@ -645,10 +649,14 @@ bool SnapshotData::FromBlob(SnapshotData* out, std::string_view in) {
645649
// Metadata
646650
uint32_t magic = r.ReadArithmetic<uint32_t>();
647651
r.Debug("Read magic %" PRIx32 "\n", magic);
648-
CHECK_EQ(magic, kMagic);
652+
if (!r.ok || magic != kMagic) {
653+
fprintf(stderr, "The startup snapshot is not a Node.js snapshot blob.\n");
654+
return false;
655+
}
649656
out->metadata = r.Read<SnapshotMetadata>();
650657
r.Debug("Read metadata\n");
651-
if (!out->Check()) {
658+
if (!r.ok || !out->Check()) {
659+
if (!r.ok) fprintf(stderr, "The startup snapshot is truncated.\n");
652660
return false;
653661
}
654662

@@ -660,13 +668,17 @@ bool SnapshotData::FromBlob(SnapshotData* out, std::string_view in) {
660668
out->code_cache = r.ReadVector<builtins::CodeCacheInfo>();
661669

662670
r.Debug("SnapshotData::FromBlob() read %d bytes\n", r.read_total);
671+
if (!r.ok) {
672+
fprintf(stderr, "The startup snapshot is truncated.\n");
673+
return false;
674+
}
663675
return true;
664676
}
665677

666678
bool SnapshotData::Check() const {
667679
if (metadata.node_version != per_process::metadata.versions.node) {
668680
fprintf(stderr,
669-
"Failed to load the startup snapshot because it was built with"
681+
"Failed to load the startup snapshot because it was built with "
670682
"Node.js version %s and the current Node.js version is %s.\n",
671683
metadata.node_version.c_str(),
672684
NODE_VERSION);
@@ -675,7 +687,7 @@ bool SnapshotData::Check() const {
675687

676688
if (metadata.node_arch != per_process::metadata.arch) {
677689
fprintf(stderr,
678-
"Failed to load the startup snapshot because it was built with"
690+
"Failed to load the startup snapshot because it was built with "
679691
"architecture %s and the architecture is %s.\n",
680692
metadata.node_arch.c_str(),
681693
NODE_ARCH);
@@ -684,7 +696,7 @@ bool SnapshotData::Check() const {
684696

685697
if (metadata.node_platform != per_process::metadata.platform) {
686698
fprintf(stderr,
687-
"Failed to load the startup snapshot because it was built with"
699+
"Failed to load the startup snapshot because it was built with "
688700
"platform %s and the current platform is %s.\n",
689701
metadata.node_platform.c_str(),
690702
NODE_PLATFORM);
Lines changed: 49 additions & 0 deletions

0 commit comments

Comments
 (0)