From 3c25ef58914e2846902b07e56f83fe26e4074d56 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 18 Sep 2026 00:05:43 +0000 Subject: [PATCH 1/3] 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 --- src/histogram.cc | 181 +++++++++++------- .../test-perf-hooks-histogram-import.js | 141 ++++++++++++++ 2 files changed, 249 insertions(+), 73 deletions(-) create mode 100644 test/parallel/test-perf-hooks-histogram-import.js diff --git a/src/histogram.cc b/src/histogram.cc index 63f297936773..474b60b9ea7b 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(std::numeric_limits::max())) { + return false; + } + *val = static_cast(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(std::numeric_limits::max())) { + return false; + } + *val = static_cast(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(u); return true; } @@ -1502,13 +1535,12 @@ std::shared_ptr 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::max(); - int figures = 3; + int32_t figures = 3; int64_t total_count = 0; int64_t min_value = std::numeric_limits::max(); int64_t max_value = 0; @@ -1517,6 +1549,20 @@ std::shared_ptr 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> sparse_counts; @@ -1530,74 +1576,49 @@ std::shared_ptr 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(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(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(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(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(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(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(std::numeric_limits::max())) - return nullptr; - norm_offset = static_cast(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(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::Import(const uint8_t* data, size_t len) { // reserve() to OOM-crash before the loop catches the error. if (arr_len > static_cast(end - p)) return nullptr; sparse_counts.reserve(static_cast(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(delta); - sparse_counts.emplace_back(acc_idx, static_cast(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::max()) return nullptr; + sparse_counts.emplace_back(static_cast(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::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(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::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::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 000000000000..e4079f2d3614 --- /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); +} From 3e0dcae977362bd73d539037268dda71b2aa64e7 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 18 Sep 2026 00:16:09 +0000 Subject: [PATCH 2/3] perf_hooks: add histogram export format version 2 Histograms exported by Node.js v26.9.0 use format version 1, which rejects unknown keys on import. Adding fields to version 1 data would therefore break importing it in v26.9.0, even though that release claims to support version 1. Introduce format version 2. It has the same layout as version 1, but unknown keys are ignored on import, so fields can be added to it later without changing the version again, while older releases reject it based on the version rather than on the new fields. `histogram.export()` now produces version 2 data. `importHistogram()` accepts versions 1 and 2, and imports version 1 data, as well as data without a version, with the original semantics. Assisted-by: OpenCode Signed-off-by: James M Snell --- doc/api/perf_hooks.md | 32 +++- src/histogram.cc | 98 +++++++++++- .../test-perf-hooks-histogram-import.js | 144 ++++++++++++++++++ 3 files changed, 266 insertions(+), 8 deletions(-) diff --git a/doc/api/perf_hooks.md b/doc/api/perf_hooks.md index 69a4d6613b9b..25027a9b686d 100644 --- a/doc/api/perf_hooks.md +++ b/doc/api/perf_hooks.md @@ -1777,6 +1777,11 @@ console.log(snapshot.percentile(99)); * `data` {Uint8Array} A CBOR-encoded histogram previously produced by @@ -1787,6 +1792,9 @@ Reconstructs a histogram from a CBOR-encoded `Uint8Array`. The returned histogram is a full {RecordableHistogram} with all bucket data, configuration, and EWMA state restored. New values can be recorded into it. +Data in any format version produced by [`histogram.export()`][] can be +imported. See [histogram export format compatibility][] for details. + ```js const { createHistogram, importHistogram } = require('node:perf_hooks'); @@ -2180,6 +2188,10 @@ loop delay threshold. * Returns: {Uint8Array} @@ -2198,7 +2210,7 @@ The CBOR payload is a map with integer keys: | Key | Type | Field | | --- | ------- | --------------------------------------------- | -| 0 | uint | Format version (currently 1) | +| 0 | uint | Format version (currently 2) | | 1 | uint | Lowest discernible value | | 2 | uint | Highest trackable value | | 3 | uint | Significant figures | @@ -2213,6 +2225,23 @@ The CBOR payload is a map with integer keys: Any standard CBOR decoder can parse the output. +#### Histogram export format compatibility + +[`perf_hooks.importHistogram()`][] accepts every format version that +`histogram.export()` has produced: + +* Version 1 was produced by Node.js v26.9.0. Data with a version 1 key, or + without a version key, is imported with the original semantics: keys + that are not listed above are rejected. +* Version 2 has the same layout as version 1. Keys that are not recognized + are ignored, so later versions of Node.js can add fields to version 2 + data without changing the version, and the data remains importable. + +Data with any other version is rejected. + +When the total count, min, or max value is absent, it is derived from the +bucket counts. A total count that is present must match the bucket counts. + ### `histogram.ewmaMean` @@ -2190,7 +2190,7 @@ loop delay threshold. added: v26.9.0 changes: - version: REPLACEME - pr-url: https://github.com/nodejs/node/pull/00000 + pr-url: https://github.com/nodejs/node/pull/66098 description: The output uses format version 2. -->