Commit f9166417f84 for nodejs
commit f9166417f8416a224e0923fc70cf6e7f442fd7ff
Author: James M Snell <jasnell@gmail.com>
Date: Fri Sep 18 00:05:43 2026 +0000
perf_hooks: harden histogram CBOR import validation
`importHistogram()` did not check the CBOR major type of keys and
integer values, so other data items were decoded as integers. It also
accepted duplicate keys, silently truncated integers cast to narrower
types, allowed bucket counts that did not fit into an `int64_t`, and
trusted the total count, min, and max, leaving imported histograms in
an inconsistent state when those were absent or did not match the
counts.
Validate major types and value ranges, reject duplicate keys and
non-increasing sparse count indexes, and derive the total count, min,
and max from the counts when they are absent. A total count that is
present must match the counts. Data produced by `histogram.export()`
is unaffected.
Assisted-by: OpenCode
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/66098
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
diff --git a/src/histogram.cc b/src/histogram.cc
index 63f29793677..474b60b9ea7 100644
--- a/src/histogram.cc
+++ b/src/histogram.cc
@@ -1066,12 +1066,45 @@ static bool CborReadFloat64(const uint8_t*& p,
return true;
}
+// Read the argument of a data item that must have the given major type
+// (kCborUint, kCborArray, or kCborMap). CborReadUint() alone decodes the
+// argument of any major type.
+static bool CborReadArgument(const uint8_t*& p,
+ const uint8_t* end,
+ uint8_t major,
+ uint64_t* val) {
+ if (p >= end || (*p & 0xe0) != major) return false;
+ return CborReadUint(p, end, val);
+}
+
+// Read an unsigned integer that must fit into a non-negative int64_t.
+static bool CborReadInt64(const uint8_t*& p, const uint8_t* end, int64_t* val) {
+ uint64_t v;
+ if (!CborReadArgument(p, end, kCborUint, &v) ||
+ v > static_cast<uint64_t>(std::numeric_limits<int64_t>::max())) {
+ return false;
+ }
+ *val = static_cast<int64_t>(v);
+ return true;
+}
+
+// Read an unsigned integer that must fit into a non-negative int32_t.
+static bool CborReadInt32(const uint8_t*& p, const uint8_t* end, int32_t* val) {
+ uint64_t v;
+ if (!CborReadArgument(p, end, kCborUint, &v) ||
+ v > static_cast<uint64_t>(std::numeric_limits<int32_t>::max())) {
+ return false;
+ }
+ *val = static_cast<int32_t>(v);
+ return true;
+}
+
// Read a value that may be either a uint or float64.
static bool CborReadNumber(const uint8_t*& p, const uint8_t* end, double* val) {
if (p >= end) return false;
if (*p == kCborFloat64) return CborReadFloat64(p, end, val);
uint64_t u;
- if (!CborReadUint(p, end, &u)) return false;
+ if (!CborReadArgument(p, end, kCborUint, &u)) return false;
*val = static_cast<double>(u);
return true;
}
@@ -1502,13 +1535,12 @@ std::shared_ptr<Histogram> Histogram::Import(const uint8_t* data, size_t len) {
const uint8_t* end = data + len;
// Read top-level map header.
- if (p >= end || (*p >> 5) != 5) return nullptr; // Must be a map.
uint64_t map_size;
- if (!CborReadUint(p, end, &map_size)) return nullptr;
+ if (!CborReadArgument(p, end, kCborMap, &map_size)) return nullptr;
int64_t lowest = 1;
int64_t highest = std::numeric_limits<int64_t>::max();
- int figures = 3;
+ int32_t figures = 3;
int64_t total_count = 0;
int64_t min_value = std::numeric_limits<int64_t>::max();
int64_t max_value = 0;
@@ -1517,6 +1549,20 @@ std::shared_ptr<Histogram> Histogram::Import(const uint8_t* data, size_t len) {
int32_t counts_len = 0;
uint64_t version = 0;
+ // Bitsets of the keys read so far, used to reject duplicate keys and to
+ // tell whether a field was present. All known keys are less than 64.
+ uint64_t seen_keys = 0;
+ uint64_t seen_ewma_keys = 0;
+ auto mark_seen = [](uint64_t* seen, uint64_t key) {
+ const uint64_t bit = uint64_t{1} << key;
+ if (*seen & bit) return false;
+ *seen |= bit;
+ return true;
+ };
+ auto has_key = [&seen_keys](uint64_t key) {
+ return (seen_keys & (uint64_t{1} << key)) != 0;
+ };
+
// Sparse counts storage.
std::vector<std::pair<int32_t, int64_t>> sparse_counts;
@@ -1530,74 +1576,49 @@ std::shared_ptr<Histogram> Histogram::Import(const uint8_t* data, size_t len) {
for (uint64_t i = 0; i < map_size; i++) {
// Read key (unsigned int).
uint64_t key;
- if (!CborReadUint(p, end, &key)) return nullptr;
+ if (!CborReadArgument(p, end, kCborUint, &key)) return nullptr;
+ if (key > kKeyEwma) return nullptr; // Unknown key.
+ if (!mark_seen(&seen_keys, key)) return nullptr; // Duplicate key.
switch (key) {
case kKeyVersion:
- if (!CborReadUint(p, end, &version)) return nullptr;
+ if (!CborReadArgument(p, end, kCborUint, &version)) return nullptr;
if (version != kExportVersion) return nullptr;
break;
- case kKeyLowest: {
- uint64_t v;
- if (!CborReadUint(p, end, &v)) return nullptr;
- lowest = static_cast<int64_t>(v);
+ case kKeyLowest:
+ if (!CborReadInt64(p, end, &lowest)) return nullptr;
break;
- }
- case kKeyHighest: {
- uint64_t v;
- if (!CborReadUint(p, end, &v)) return nullptr;
- highest = static_cast<int64_t>(v);
+ case kKeyHighest:
+ if (!CborReadInt64(p, end, &highest)) return nullptr;
break;
- }
- case kKeyFigures: {
- uint64_t v;
- if (!CborReadUint(p, end, &v)) return nullptr;
- figures = static_cast<int>(v);
+ case kKeyFigures:
+ if (!CborReadInt32(p, end, &figures)) return nullptr;
break;
- }
- case kKeyTotalCount: {
- uint64_t v;
- if (!CborReadUint(p, end, &v)) return nullptr;
- total_count = static_cast<int64_t>(v);
+ case kKeyTotalCount:
+ if (!CborReadInt64(p, end, &total_count)) return nullptr;
break;
- }
- case kKeyMin: {
- uint64_t v;
- if (!CborReadUint(p, end, &v)) return nullptr;
- min_value = static_cast<int64_t>(v);
+ case kKeyMin:
+ if (!CborReadInt64(p, end, &min_value)) return nullptr;
break;
- }
- case kKeyMax: {
- uint64_t v;
- if (!CborReadUint(p, end, &v)) return nullptr;
- max_value = static_cast<int64_t>(v);
+ case kKeyMax:
+ if (!CborReadInt64(p, end, &max_value)) return nullptr;
break;
- }
- case kKeyNormOffset: {
- uint64_t v;
- if (!CborReadUint(p, end, &v)) return nullptr;
- // Reject values that cannot be represented as int32_t; the
- // static_cast below would wrap and produce an arbitrary offset.
- if (v > static_cast<uint64_t>(std::numeric_limits<int32_t>::max()))
- return nullptr;
- norm_offset = static_cast<int32_t>(v);
+ case kKeyNormOffset:
+ // Reject values that cannot be represented as int32_t; casting them
+ // would wrap and produce an arbitrary offset.
+ if (!CborReadInt32(p, end, &norm_offset)) return nullptr;
break;
- }
case kKeyConvRatio:
if (!CborReadNumber(p, end, &conv_ratio)) return nullptr;
break;
- case kKeyCountsLen: {
- uint64_t v;
- if (!CborReadUint(p, end, &v)) return nullptr;
- counts_len = static_cast<int32_t>(v);
+ case kKeyCountsLen:
+ if (!CborReadInt32(p, end, &counts_len)) return nullptr;
break;
- }
case kKeyCounts: {
// Array of flat [delta, count, ...] pairs. Indices are
// delta-encoded: accumulate to recover absolute indices.
- if (p >= end || (*p >> 5) != 4) return nullptr;
uint64_t arr_len;
- if (!CborReadUint(p, end, &arr_len)) return nullptr;
+ if (!CborReadArgument(p, end, kCborArray, &arr_len)) return nullptr;
if (arr_len % 2 != 0) return nullptr;
// Each element needs at least 1 byte of CBOR encoding, so
// arr_len can't exceed the remaining buffer. Without this
@@ -1605,24 +1626,30 @@ std::shared_ptr<Histogram> Histogram::Import(const uint8_t* data, size_t len) {
// reserve() to OOM-crash before the loop catches the error.
if (arr_len > static_cast<uint64_t>(end - p)) return nullptr;
sparse_counts.reserve(static_cast<size_t>(arr_len / 2));
- int32_t acc_idx = 0;
+ int64_t acc_idx = 0;
for (uint64_t j = 0; j < arr_len; j += 2) {
- uint64_t delta, cnt;
- if (!CborReadUint(p, end, &delta)) return nullptr;
- if (!CborReadUint(p, end, &cnt)) return nullptr;
- acc_idx += static_cast<int32_t>(delta);
- sparse_counts.emplace_back(acc_idx, static_cast<int64_t>(cnt));
+ int32_t delta;
+ int64_t cnt;
+ if (!CborReadInt32(p, end, &delta)) return nullptr;
+ if (!CborReadInt64(p, end, &cnt)) return nullptr;
+ // Indices are strictly increasing, so only the first delta (the
+ // absolute index of the first non-empty bucket) may be zero.
+ if (j > 0 && delta == 0) return nullptr;
+ acc_idx += delta;
+ if (acc_idx > std::numeric_limits<int32_t>::max()) return nullptr;
+ sparse_counts.emplace_back(static_cast<int32_t>(acc_idx), cnt);
}
break;
}
case kKeyEwma: {
// Sub-map for EWMA state.
- if (p >= end || (*p >> 5) != 5) return nullptr;
uint64_t sub_size;
- if (!CborReadUint(p, end, &sub_size)) return nullptr;
+ if (!CborReadArgument(p, end, kCborMap, &sub_size)) return nullptr;
for (uint64_t j = 0; j < sub_size; j++) {
uint64_t sub_key;
- if (!CborReadUint(p, end, &sub_key)) return nullptr;
+ if (!CborReadArgument(p, end, kCborUint, &sub_key)) return nullptr;
+ if (sub_key > kEwmaThreshold) return nullptr; // Unknown EWMA key.
+ if (!mark_seen(&seen_ewma_keys, sub_key)) return nullptr;
switch (sub_key) {
case kEwmaAlpha:
if (!CborReadNumber(p, end, &ewma_alpha)) return nullptr;
@@ -1636,20 +1663,13 @@ std::shared_ptr<Histogram> Histogram::Import(const uint8_t* data, size_t len) {
case kEwmaErrorRate:
if (!CborReadNumber(p, end, &ewma_error_rate)) return nullptr;
break;
- case kEwmaThreshold: {
- uint64_t v;
- if (!CborReadUint(p, end, &v)) return nullptr;
- threshold = static_cast<int64_t>(v);
+ case kEwmaThreshold:
+ if (!CborReadInt64(p, end, &threshold)) return nullptr;
break;
- }
- default:
- return nullptr; // Unknown EWMA key.
}
}
break;
}
- default:
- return nullptr; // Unknown key.
}
}
@@ -1679,16 +1699,31 @@ std::shared_ptr<Histogram> Histogram::Import(const uint8_t* data, size_t len) {
if (norm_offset < 0 || norm_offset >= counts_len) return nullptr;
// Restore counts directly.
+ int64_t observed_total_count = 0;
for (const auto& [idx, cnt] : sparse_counts) {
if (idx < 0 || idx >= counts_len) return nullptr;
+ // The counts must add up without overflowing int64_t.
+ if (cnt > std::numeric_limits<int64_t>::max() - observed_total_count) {
+ return nullptr;
+ }
+ observed_total_count += cnt;
histogram->histogram_->counts[idx] = cnt;
}
- histogram->histogram_->total_count = total_count;
- histogram->histogram_->min_value = min_value;
- histogram->histogram_->max_value = max_value;
histogram->histogram_->normalizing_index_offset = norm_offset;
histogram->histogram_->conversion_ratio = conv_ratio;
+ // Derive the total count, min, and max from the counts, as
+ // Histogram::Subtract() does. This keeps the histogram consistent when
+ // any of them are absent. A total count that is present must match the
+ // counts. Min and max values that are present are restored as recorded.
+ hdr_reset_internal_counters(histogram->histogram_.get());
+ if (has_key(kKeyTotalCount) &&
+ total_count != histogram->histogram_->total_count) {
+ return nullptr;
+ }
+ if (has_key(kKeyMin)) histogram->histogram_->min_value = min_value;
+ if (has_key(kKeyMax)) histogram->histogram_->max_value = max_value;
+
// Restore EWMA state.
if (ewma_alpha > 0) {
histogram->ewma_mean_ = ewma_mean;
diff --git a/test/parallel/test-perf-hooks-histogram-import.js b/test/parallel/test-perf-hooks-histogram-import.js
new file mode 100644
index 00000000000..e4079f2d361
--- /dev/null
+++ b/test/parallel/test-perf-hooks-histogram-import.js
@@ -0,0 +1,141 @@
+'use strict';
+
+// Tests validation of the CBOR payload accepted by importHistogram().
+
+require('../common');
+const assert = require('node:assert');
+const { createHistogram, importHistogram } = require('node:perf_hooks');
+
+// Minimal CBOR (RFC 8949) encoding helpers for building payloads. Each
+// helper returns an array of bytes.
+function head(major, value) {
+ const v = BigInt(value);
+ const m = major << 5;
+ if (v < 24n) return [m | Number(v)];
+ const bytes = [];
+ const width = v < 0x100n ? 1 : v < 0x10000n ? 2 : v < 0x100000000n ? 4 : 8;
+ for (let i = width - 1; i >= 0; i--) {
+ bytes.push(Number((v >> BigInt(i * 8)) & 0xffn));
+ }
+ return [m | { 1: 24, 2: 25, 4: 26, 8: 27 }[width], ...bytes];
+}
+const uint = (value) => head(0, value);
+const negint = (value) => head(1, value); // Encodes -1 - value.
+const text = (str) => [...head(3, Buffer.byteLength(str)), ...Buffer.from(str)];
+function f64(value) {
+ const buf = Buffer.alloc(9);
+ buf[0] = 0xfb;
+ buf.writeDoubleBE(value, 1);
+ return [...buf];
+}
+
+function array(items) {
+ const out = head(4, items.length);
+ for (const item of items) out.push(...item);
+ return out;
+}
+
+function map(entries) {
+ const out = head(5, entries.length);
+ for (const { 0: key, 1: value } of entries) out.push(...key, ...value);
+ return out;
+}
+const importBytes = (bytes) => importHistogram(new Uint8Array(bytes));
+const kInvalid = { code: 'ERR_INVALID_ARG_VALUE' };
+
+// lowest=1 (the default), highest=100, figures=1 produces counts_len=64, with
+// indexes below 32 mapping to the identical values.
+const kLayout = [
+ [uint(2), uint(100)], // highest
+ [uint(3), uint(1)], // figures
+ [uint(9), uint(64)], // counts length
+];
+const counts = (...pairs) => [uint(10), array(pairs.map((v) => uint(v)))];
+
+{
+ // Absent total count, min, and max are derived from the counts.
+ const h = importBytes(map([...kLayout, counts(5, 2, 3, 1)]));
+ assert.strictEqual(h.count, 3);
+ assert.strictEqual(h.min, 5);
+ assert.strictEqual(h.max, 8);
+ assert.strictEqual(h.percentile(50), 5);
+ assert.strictEqual(h.percentile(100), 8);
+}
+
+{
+ // A total count that is present must match the counts.
+ const h = importBytes(map([...kLayout, [uint(4), uint(3)], counts(5, 2, 3, 1)]));
+ assert.strictEqual(h.count, 3);
+ assert.throws(
+ () => importBytes(map([...kLayout, [uint(4), uint(4)], counts(5, 2, 3, 1)])),
+ kInvalid);
+ assert.throws(
+ () => importBytes(map([...kLayout, [uint(4), uint(1)]])),
+ kInvalid);
+}
+
+{
+ // Min and max values that are present are restored as recorded.
+ const h = createHistogram();
+ h.record(987654321);
+ h.record(1234567891);
+ const h2 = importHistogram(h.export());
+ assert.strictEqual(h2.min, h.min);
+ assert.strictEqual(h2.max, h.max);
+}
+
+// Duplicate keys are rejected.
+assert.throws(() => importBytes(map([...kLayout, [uint(3), uint(1)]])),
+ kInvalid);
+assert.throws(
+ () => importBytes(map([...kLayout, counts(5, 2), counts(6, 7)])),
+ kInvalid);
+assert.throws(() => importBytes(map([
+ ...kLayout,
+ [uint(11), map([[uint(0), f64(0.5)], [uint(0), f64(0.5)]])],
+])), kInvalid);
+
+// Keys must be unsigned integers.
+assert.throws(() => importBytes(map([...kLayout, [text('a'), uint(1)]])),
+ kInvalid);
+assert.throws(() => importBytes(map([...kLayout, [negint(0), uint(1)]])),
+ kInvalid);
+
+// Values must have the expected CBOR types.
+assert.throws(() => importBytes(map([
+ [uint(2), uint(100)], [uint(3), text('1')], [uint(9), uint(64)],
+])), kInvalid);
+assert.throws(() => importBytes(map([
+ [uint(2), negint(99)], [uint(3), uint(1)], [uint(9), uint(64)],
+])), kInvalid);
+assert.throws(() => importBytes(map([...kLayout, [uint(10), map([])]])),
+ kInvalid);
+assert.throws(() => importBytes(map([...kLayout, [uint(11), array([])]])),
+ kInvalid);
+
+// Values must not be truncated when casting to the field's type.
+assert.throws(() => importBytes(map([
+ [uint(2), uint(100)], [uint(3), uint(1)], [uint(9), uint(2n ** 32n + 64n)],
+])), kInvalid);
+assert.throws(() => importBytes(map([
+ [uint(2), uint(100)], [uint(3), uint(2n ** 32n + 1n)], [uint(9), uint(64)],
+])), kInvalid);
+assert.throws(() => importBytes(map([...kLayout, counts(5, 2n ** 63n)])),
+ kInvalid);
+assert.throws(() => importBytes(map([...kLayout, counts(2n ** 31n, 1)])),
+ kInvalid);
+
+// Counts must not overflow when added up.
+assert.throws(() => importBytes(map([
+ ...kLayout,
+ counts(1, 2n ** 62n, 1, 2n ** 62n, 1, 2n ** 62n),
+])), kInvalid);
+
+{
+ // Sparse count indexes must be strictly increasing. Only the first delta,
+ // which is an absolute index, may be zero.
+ const h = importBytes(map([...kLayout, counts(0, 1, 5, 1)]));
+ assert.strictEqual(h.count, 2);
+ assert.throws(() => importBytes(map([...kLayout, counts(5, 2, 0, 1)])),
+ kInvalid);
+}