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>

Author: codebytere

src/blob_serializer_deserializer-inl.h

@@ -111,6 +111,11 @@ std::vector<T> BlobDeserializer<Impl>::ReadVector() {
   if (count == 0) {
     return std::vector<T>();
   }
+  // Every element takes at least one byte, so this bounds the allocation.
+  if (count > sink.size() - read_total) {
+    ok = false;
+    return std::vector<T>();
+  }
   if (is_debug) {
     Debug("Reading %d vector elements...\n", count);
   }
@@ -143,6 +148,10 @@ std::string_view BlobDeserializer<Impl>::ReadStringView(StringLogMode mode) {
     Debug("ReadStringView() read an empty view\n");
     return std::string_view();
   }
+  if (length > sink.size() - read_total) {
+    ok = false;
+    return std::string_view();
+  }
 
   std::string_view result(sink.data() + read_total, length);
   Debug("%p, read %zu bytes", result.data(), result.size());
@@ -167,6 +176,11 @@ void BlobDeserializer<Impl>::ReadArithmetic(T* out, size_t count) {
   }
 
   size_t size = sizeof(T) * count;
+  if (!ok || count > (sink.size() - read_total) / sizeof(T)) {
+    ok = false;
+    memset(out, 0, size);
+    return;
+  }
   memcpy(out, sink.data() + read_total, size);
 
   if (is_debug) {

src/blob_serializer_deserializer.h

@@ -45,6 +45,9 @@ class BlobDeserializer : public BlobSerializerDeserializer {
 
   size_t read_total = 0;
   std::string_view sink;
+  // Cleared when a read would go past the end of `sink`; that read and all
+  // later ones yield zeroes and empty views, so callers can check at the end.
+  bool ok = true;
 
   Impl* impl() { return static_cast<Impl*>(this); }
   const Impl* impl() const { return static_cast<const Impl*>(this); }

src/node_file_utils.cc

@@ -240,6 +240,7 @@ std::vector<char> ReadFileSync(FILE* fp) {
   CHECK_EQ(err, 0);
 
   std::vector<char> contents(size);
+  if (size == 0) return contents;
   size_t num_read = fread(contents.data(), size, 1, fp);
   CHECK_EQ(num_read, 1);
   return contents;

src/node_snapshotable.cc

@@ -187,12 +187,16 @@ v8::StartupData SnapshotDeserializer::Read() {
   int raw_size = ReadArithmetic<int>();
   Debug("size=%d\n", raw_size);
 
-  CHECK_GT(raw_size, 0);  // There should be no startup data of size 0.
+  if (raw_size <= 0 ||
+      static_cast<size_t>(raw_size) > sink.size() - read_total) {
+    ok = false;
+    return v8::StartupData{nullptr, 0};
+  }
   // The data pointer of v8::StartupData would be deleted so it must be new'ed.
-  std::unique_ptr<char> buf = std::unique_ptr<char>(new char[raw_size]);
-  ReadArithmetic<char>(buf.get(), raw_size);
+  char* buf = new char[raw_size];
+  ReadArithmetic<char>(buf, raw_size);
 
-  return v8::StartupData{buf.release(), raw_size};
+  return v8::StartupData{buf, raw_size};
 }
 
 template <>
@@ -646,10 +650,14 @@ bool SnapshotData::FromBlob(SnapshotData* out, std::string_view in) {
   // Metadata
   uint32_t magic = r.ReadArithmetic<uint32_t>();
   r.Debug("Read magic %" PRIx32 "\n", magic);
-  CHECK_EQ(magic, kMagic);
+  if (!r.ok || magic != kMagic) {
+    fprintf(stderr, "The startup snapshot is not a Node.js snapshot blob.\n");
+    return false;
+  }
   out->metadata = r.Read<SnapshotMetadata>();
   r.Debug("Read metadata\n");
-  if (!out->Check()) {
+  if (!r.ok || !out->Check()) {
+    if (!r.ok) fprintf(stderr, "The startup snapshot is truncated.\n");
     return false;
   }
 
@@ -661,13 +669,17 @@ bool SnapshotData::FromBlob(SnapshotData* out, std::string_view in) {
   out->code_cache = r.ReadVector<builtins::CodeCacheInfo>();
 
   r.Debug("SnapshotData::FromBlob() read %d bytes\n", r.read_total);
+  if (!r.ok) {
+    fprintf(stderr, "The startup snapshot is truncated.\n");
+    return false;
+  }
   return true;
 }
 
 bool SnapshotData::Check() const {
   if (metadata.node_version != per_process::metadata.versions.node) {
     fprintf(stderr,
-            "Failed to load the startup snapshot because it was built with"
+            "Failed to load the startup snapshot because it was built with "
             "Node.js version %s and the current Node.js version is %s.\n",
             metadata.node_version.c_str(),
             NODE_VERSION);
@@ -676,7 +688,7 @@ bool SnapshotData::Check() const {
 
   if (metadata.node_arch != per_process::metadata.arch) {
     fprintf(stderr,
-            "Failed to load the startup snapshot because it was built with"
+            "Failed to load the startup snapshot because it was built with "
             "architecture %s and the architecture is %s.\n",
             metadata.node_arch.c_str(),
             NODE_ARCH);
@@ -685,7 +697,7 @@ bool SnapshotData::Check() const {
 
   if (metadata.node_platform != per_process::metadata.platform) {
     fprintf(stderr,
-            "Failed to load the startup snapshot because it was built with"
+            "Failed to load the startup snapshot because it was built with "
             "platform %s and the current platform is %s.\n",
             metadata.node_platform.c_str(),
             NODE_PLATFORM);

test/parallel/test-snapshot-invalid-blob.js

@@ -0,0 +1,49 @@
+'use strict';
+
+// This tests that Node.js reports an error, rather than crashing, when the
+// file passed to --snapshot-blob is empty, is not a snapshot, or is truncated.
+
+require('../common');
+const {
+  spawnSyncAndExit,
+  spawnSyncAndExitWithoutError,
+} = require('../common/child_process');
+const tmpdir = require('../common/tmpdir');
+const fixtures = require('../common/fixtures');
+const fs = require('fs');
+
+tmpdir.refresh();
+const entry = fixtures.path('empty.js');
+
+function expectFailure(blobPath, stderr) {
+  spawnSyncAndExit(process.execPath, ['--snapshot-blob', blobPath, entry], {
+    cwd: tmpdir.path,
+  }, {
+    status: 14,
+    signal: null,
+    stderr,
+  });
+}
+
+{
+  const blobPath = tmpdir.resolve('empty.blob');
+  fs.writeFileSync(blobPath, '');
+  expectFailure(blobPath, /not a Node\.js snapshot blob/);
+}
+
+{
+  const blobPath = tmpdir.resolve('garbage.blob');
+  fs.writeFileSync(blobPath, Buffer.alloc(4096, 0x61));
+  expectFailure(blobPath, /not a Node\.js snapshot blob/);
+}
+
+{
+  const blobPath = tmpdir.resolve('snapshot.blob');
+  spawnSyncAndExitWithoutError(process.execPath, [
+    '--snapshot-blob', blobPath, '--build-snapshot', entry,
+  ], { cwd: tmpdir.path });
+  const blob = fs.readFileSync(blobPath);
+  const truncatedPath = tmpdir.resolve('truncated.blob');
+  fs.writeFileSync(truncatedPath, blob.subarray(0, blob.length >> 1));
+  expectFailure(truncatedPath, /truncated/);
+}