diff --git a/src/blob_serializer_deserializer-inl.h b/src/blob_serializer_deserializer-inl.h index 2a2aed1171f7..876c79578302 100644 --- a/src/blob_serializer_deserializer-inl.h +++ b/src/blob_serializer_deserializer-inl.h @@ -111,6 +111,11 @@ std::vector BlobDeserializer::ReadVector() { if (count == 0) { return std::vector(); } + // Every element takes at least one byte, so this bounds the allocation. + if (count > sink.size() - read_total) { + ok = false; + return std::vector(); + } if (is_debug) { Debug("Reading %d vector elements...\n", count); } @@ -143,6 +148,10 @@ std::string_view BlobDeserializer::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::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) { diff --git a/src/blob_serializer_deserializer.h b/src/blob_serializer_deserializer.h index cd63d0477675..e4d1efe99f53 100644 --- a/src/blob_serializer_deserializer.h +++ b/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(this); } const Impl* impl() const { return static_cast(this); } diff --git a/src/node_file_utils.cc b/src/node_file_utils.cc index 5f0ef1492135..ced08b1d0ff2 100644 --- a/src/node_file_utils.cc +++ b/src/node_file_utils.cc @@ -240,6 +240,7 @@ std::vector ReadFileSync(FILE* fp) { CHECK_EQ(err, 0); std::vector contents(size); + if (size == 0) return contents; size_t num_read = fread(contents.data(), size, 1, fp); CHECK_EQ(num_read, 1); return contents; diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 37123682dd16..6d6961dd68b7 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -186,12 +186,16 @@ v8::StartupData SnapshotDeserializer::Read() { int raw_size = ReadArithmetic(); 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(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 buf = std::unique_ptr(new char[raw_size]); - ReadArithmetic(buf.get(), raw_size); + char* buf = new char[raw_size]; + ReadArithmetic(buf, raw_size); - return v8::StartupData{buf.release(), raw_size}; + return v8::StartupData{buf, raw_size}; } template <> @@ -645,10 +649,14 @@ bool SnapshotData::FromBlob(SnapshotData* out, std::string_view in) { // Metadata uint32_t magic = r.ReadArithmetic(); 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(); 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; } @@ -660,13 +668,17 @@ bool SnapshotData::FromBlob(SnapshotData* out, std::string_view in) { out->code_cache = r.ReadVector(); 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); @@ -675,7 +687,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); @@ -684,7 +696,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); diff --git a/test/parallel/test-snapshot-invalid-blob.js b/test/parallel/test-snapshot-invalid-blob.js new file mode 100644 index 000000000000..9157a2d3a482 --- /dev/null +++ b/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/); +}