Skip to content

Commit 4f93644

Browse files
authored
fix: close review issues #25696-#25703 on 3.4.0 (#25695)
* fix: close review issues #25696-#25703 on 3.4.0 Rebase the OPEN review follow-ups onto live upstream master (c0f78a1, 3.4.0) so Stream drain/backpressure from #43 is kept. Pin nan 2.28.0 and c8 12.0.0; CI uses npm ci on Node 22/24 with full-SHA GitHub Actions. engines.node is >=22. Cap Stream receive before allocate and drop buf on close/end/error. Re-emit underlying socket errors when the Stream is not already dead so a socket 'error' is not swallowed by the listener. Apply kMaxContainer to pack array/map walks. Cap CLI stdin concat at MAX_STDIN_BYTES. Document a risk-based bump window in SECURITY.md. * fix: drop Node 24 from engines and CI InitLazy still calls ObjectTemplate::SetIndexedPropertyHandler, which Node 24 V8 headers no longer provide. Parent never tested 24. engines.node is ^22 so 24 is not advertised. CI matrix is [22] only. * fix: cap PackArrayInterpreted at kMaxContainer Snapshot Length() once, throw when n > 1e6, and iterate the frozen n so interpret cannot walk a sparse array past the PackArray policy.
1 parent c0f78a1 commit 4f93644

14 files changed

Lines changed: 302 additions & 48 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -11,41 +11,36 @@ jobs:
1111
fail-fast: false
1212
matrix:
1313
# windows-2022 ships VS 2022. windows-latest currently has VS 2026
14-
# (18.x), which node-gyp 10/11 bundled with Node 18/20/22 reports as
14+
# (18.x), which node-gyp 10/11 bundled with Node 22 reports as
1515
# unknown version "undefined" and then fails the native rebuild.
1616
os: [ubuntu-latest, macos-latest, windows-2022]
17-
node: [18, 20, 22]
17+
node: [22]
1818
steps:
19-
- uses: actions/checkout@v4
20-
- uses: actions/setup-node@v4
19+
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
20+
- uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0
2121
with:
2222
node-version: ${{ matrix.node }}
2323
- name: Install and test
2424
# bash so `&&` is a hard stop on Windows too (PowerShell would still
2525
# run npm test after a failed rebuild).
2626
shell: bash
27-
run: npm install && npm test
27+
run: npm ci && npm test
2828

2929
coverage:
3030
runs-on: ubuntu-latest
3131
steps:
32-
- uses: actions/checkout@v4
33-
- uses: actions/setup-node@v4
32+
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
33+
- uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0
3434
with:
35-
node-version: 20
36-
- uses: actions/setup-python@v5
35+
node-version: 22
36+
- uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0
3737
with:
3838
python-version: '3.x'
3939
- name: Install gcovr
4040
run: |
4141
gcovr --version || pip install --upgrade gcovr
4242
- name: Install dependencies
43-
run: |
44-
if [ -f package-lock.json ]; then
45-
npm ci || npm install
46-
else
47-
npm install
48-
fi
43+
run: npm ci
4944
# Fails the job when JS or native coverage drops below 95%.
5045
- name: Coverage
5146
run: npm run coverage

‎README.md‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
and de-serializes JavaScript values with [MessagePack](https://msgpack.org).
33
Packed output is a `Buffer` and is typically much smaller than JSON.
44

5-
Version 3.3 requires **Node.js 18+**, vendors **msgpack-c c-7.0.2**, unpacks
5+
Version 3.4 requires **Node.js 22.x**, vendors **msgpack-c c-7.0.2**, unpacks
66
64-bit integers outside `Number.MAX_SAFE_INTEGER` as `bigint`, accepts
77
optional pack type/family hints, can unpack maps and arrays lazily
88
(`unpack(buf, { lazy: true })`), and applies write backpressure on
@@ -27,7 +27,9 @@ and returns a JavaScript value, or `null` if the buffer is a truncated
2727
(incomplete) MessagePack object. Oversized array/map/string bombs throw.
2828

2929
A streaming helper wraps a readable socket and emits `msg`, plus `error` when
30-
a packet cannot be unpacked (the offending buffer is dropped). `send()` packs
30+
a packet cannot be unpacked, the receive buffer would exceed
31+
`MAX_STREAM_BYTES` (the offending buffer is dropped, and the socket is
32+
destroyed when possible), or the underlying stream errors. `send()` packs
3133
and writes; it returns the boolean from the underlying `write()`, or `false`
3234
if the message was queued because a previous write returned `false` and
3335
`drain` has not fired yet. `drain` is re-emitted from the underlying
@@ -147,26 +149,33 @@ Default packing is unchanged when no recognized options object is passed.
147149

148150
### Limits
149151

150-
* array/map length ≤ 1,000,000
152+
* array/map length ≤ 1,000,000 on both pack and unpack
151153
* str/bin/ext length ≤ 32 MiB
152154
* nesting depth ≤ 512 on both pack and unpack
155+
* Stream receive buffer ≤ `MAX_STREAM_BYTES` (32 MiB + 16 bytes of framing)
156+
* CLI stdin ≤ `MAX_STDIN_BYTES` (32 MiB) before concat/parse
153157

154158
The payload `dd ff 00 00 00` throws `msgpack unpack limit exceeded`. Packing a
155-
value nested deeper than 512 throws `Cowardly refusing to pack object nested
156-
more than 512 levels deep` instead of overflowing the C stack.
159+
sparse array or map whose length exceeds 1,000,000 throws
160+
`msgpack pack limit exceeded`. Packing a value nested deeper than 512 throws
161+
`Cowardly refusing to pack object nested more than 512 levels deep` instead of
162+
overflowing the C stack. Incomplete Stream frames that would grow past
163+
`MAX_STREAM_BYTES` throw `msgpack stream limit exceeded` before allocate.
157164

158165
### Building, installation, testing
159166

160167
```
161-
npm install
168+
npm ci
162169
npm test
163170
npm run coverage
164171
```
165172

166-
Needs a C/C++ toolchain and Python (node-gyp). GitHub Actions runs Node 18/20/22
167-
on Ubuntu and macOS. `npm run coverage` instruments JavaScript with c8 and the
168-
native addon with gcov, and fails under 95%. Gates and remaining uncovered
169-
lines are documented in [`COVERAGE.md`](COVERAGE.md).
173+
Needs a C/C++ toolchain and Python (node-gyp). GitHub Actions runs Node 22
174+
on Ubuntu, macOS, and windows-2022. Node 24 is not advertised: lazy unpack
175+
still uses `SetIndexedPropertyHandler`, which Node 24 V8 removed.
176+
`npm run coverage` instruments JavaScript with c8 and the native addon with
177+
gcov, and fails under 95%. Gates and remaining uncovered lines are
178+
documented in [`COVERAGE.md`](COVERAGE.md).
170179

171180
### Command Line Utilities
172181

@@ -202,7 +211,8 @@ echo '{"hello":"world"}' | bin/json2msgpack | bin/msgpack2json
202211
```
203212

204213
`msgpack2json` prints one JSON value per line and consumes every complete
205-
message in its input. Both exit non-zero on invalid or truncated input.
214+
message in its input. Both exit non-zero on invalid or truncated input, and
215+
both refuse stdin larger than `MAX_STDIN_BYTES` (32 MiB).
206216

207217
### Benchmarks
208218

‎SECURITY.md‎

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# Security notes (node-msgpack 2.0.0)
1+
# Security notes (node-msgpack 3.4.0)
22

33
This package vendors [msgpack-c](https://github.com/msgpack/msgpack-c) **c-7.0.2**
44
(`e17beb371b59459a13b48e166a11e123bda5bf93`), the C library.
@@ -50,6 +50,43 @@ on circular refs / unencodable values without freeing it. The sbuffer is now
5050
owned by an RAII guard that returns pooled buffers or `msgpack_sbuffer_free`s
5151
on every exit path, including C++ exceptions.
5252

53+
## Stream receive buffer
54+
55+
`msgpack.Stream` concatenates incomplete frames into `self.buf`. Before any
56+
allocate, a new chunk is rejected when `self.buf.length + chunk.length` would
57+
exceed `MAX_STREAM_BYTES` (32 MiB plus 16 bytes of MessagePack framing). The
58+
buffer is dropped, `'error'` is emitted (`msgpack stream limit exceeded`),
59+
and the underlying stream is `destroy()`ed when that method exists. `close`,
60+
`end`, and `error` on the underlying stream also drop `self.buf`.
61+
62+
## CLI stdin
63+
64+
`bin/json2msgpack` and `bin/msgpack2json` refuse stdin larger than
65+
`MAX_STDIN_BYTES` (32 MiB) before `Buffer.concat` / `JSON.parse` /
66+
`unpack`. They exit 1 with `stdin exceeds MAX_STDIN_BYTES`.
67+
68+
## Pack container size
69+
70+
`pack()` rejects arrays and maps whose own length exceeds 1,000,000
71+
(`kMaxContainer`), the same policy as unpack. Sparse `Array.length` above
72+
that cap throws `msgpack pack limit exceeded` without walking the holes.
73+
74+
## Dependency bump window
75+
76+
The pins below are inventory, not a calendar SLA:
77+
78+
- msgpack-c **c-7.0.2** (`e17beb371b59459a13b48e166a11e123bda5bf93`)
79+
- NAN **2.28.0** (compile-in; exact in `package.json` / shrinkwrap)
80+
81+
Risk-based window:
82+
83+
- **Critical or high** native advisories in vendored msgpack-c or in
84+
compile-in NAN: bump to a reviewed fix, or document why the pin stays,
85+
within **14 days** of that fix being available.
86+
- **Medium**: next minor of this package.
87+
- **Low / no advisory**: no scheduled bump. The inventory pin is not a
88+
commitment to track upstream on a calendar.
89+
5390
## License
5491

5592
First-party code in this repository is BSD-3-Clause (see `LICENSE`). The

‎bin/json2msgpack‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,18 @@
55

66
const msgpack = require('../lib/msgpack');
77

8+
const MAX_STDIN_BYTES = msgpack.MAX_STDIN_BYTES;
89
const chunks = [];
10+
let n = 0;
911

10-
process.stdin.on('data', (d) => chunks.push(d));
12+
process.stdin.on('data', (d) => {
13+
n += d.length;
14+
if (n > MAX_STDIN_BYTES) {
15+
console.error('json2msgpack: stdin exceeds MAX_STDIN_BYTES (' + MAX_STDIN_BYTES + ')');
16+
process.exit(1);
17+
}
18+
chunks.push(d);
19+
});
1120

1221
process.stdin.on('end', () => {
1322
const text = Buffer.concat(chunks).toString('utf8');

‎bin/msgpack2json‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,18 @@
55

66
const msgpack = require('../lib/msgpack');
77

8+
const MAX_STDIN_BYTES = msgpack.MAX_STDIN_BYTES;
89
const chunks = [];
10+
let n = 0;
911

10-
process.stdin.on('data', (d) => chunks.push(d));
12+
process.stdin.on('data', (d) => {
13+
n += d.length;
14+
if (n > MAX_STDIN_BYTES) {
15+
console.error('msgpack2json: stdin exceeds MAX_STDIN_BYTES (' + MAX_STDIN_BYTES + ')');
16+
process.exit(1);
17+
}
18+
chunks.push(d);
19+
});
1120

1221
process.stdin.on('end', () => {
1322
let buf = Buffer.concat(chunks);

‎index.d.ts‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,8 @@ export interface PackOptions {
4646
* Serialize values to MessagePack.
4747
*
4848
* A single argument is packed as itself; two or more are packed as an array
49-
* of that many elements.
49+
* of that many elements. Arrays and maps whose length exceeds 1,000,000
50+
* throw `msgpack pack limit exceeded`.
5051
*
5152
* When the second argument own-enumerates only `type`, `family`, and/or
5253
* `interpret`, it is pack options rather than a second value. `type` forces
@@ -96,14 +97,30 @@ export namespace unpack {
9697
let bytes_remaining: number;
9798
}
9899

100+
/**
101+
* Cap on `Stream` receive concatenation (32 MiB plus 16 bytes of framing).
102+
* A chunk that would take `buf.length + chunk.length` past this is rejected
103+
* before allocate.
104+
*/
105+
export const MAX_STREAM_BYTES: number;
106+
107+
/**
108+
* Cap on CLI stdin accumulation in `json2msgpack` / `msgpack2json` (32 MiB).
109+
* Overflow exits 1 before concat/parse.
110+
*/
111+
export const MAX_STDIN_BYTES: number;
112+
99113
/**
100114
* Frames MessagePack messages over a stream.
101115
*
102116
* Emits `'msg'` with each decoded value, `'drain'` when the underlying
103117
* writable is ready for more data and the send queue is empty, and
104-
* `'error'` if a packet cannot be decoded (the buffered data is then
105-
* dropped) or if queued sends are discarded because the underlying stream
106-
* emitted `error`/`close`/`end`.
118+
* `'error'` if a packet cannot be decoded, if the receive buffer would
119+
* exceed `MAX_STREAM_BYTES` (the buffered data is then dropped; the
120+
* underlying stream is destroyed when possible), if queued sends are
121+
* discarded because the underlying stream emitted `error`/`close`/`end`,
122+
* or if the underlying stream errors. `buf` is also dropped on the
123+
* underlying stream's `close` / `end` / `error`.
107124
*/
108125
export class Stream extends EventEmitter {
109126
constructor(s: NodeJS.ReadWriteStream);

‎lib/msgpack.js‎

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,12 @@ const mpBindings = require(__dirname + '/../build/Release/msgpackBinding');
1111
const bpack = mpBindings.pack;
1212
const rawUnpack = mpBindings.unpack;
1313

14+
/* 32 MiB payload (same as unpack str/bin) plus 16 bytes of MessagePack
15+
* framing so a max-legal bin32/str32 still fits in one Stream buffer. */
16+
const MAX_STREAM_BYTES = 32 * 1024 * 1024 + 16;
17+
/* CLI stdin is capped at 32 MiB before concat/parse. */
18+
const MAX_STDIN_BYTES = 32 * 1024 * 1024;
19+
1420
/* No JS pre-pass: the binding already applies toJSON at every level and
1521
* packs Dates as ISO strings. Calling toJSON here instead turned a top-level
1622
* Buffer into Buffer.prototype.toJSON's {type,data} map rather than bin. */
@@ -32,6 +38,22 @@ function Stream(s) {
3238
const self = this;
3339
events.EventEmitter.call(self);
3440
self.buf = null;
41+
let dead = false;
42+
43+
function dropBuf() {
44+
self.buf = null;
45+
}
46+
47+
function rejectLimit() {
48+
if (dead) return;
49+
dead = true;
50+
dropBuf();
51+
const err = new Error('msgpack stream limit exceeded');
52+
self.emit('error', err);
53+
if (typeof s.destroy === 'function') {
54+
s.destroy(err);
55+
}
56+
}
3557

3658
const queue = [];
3759
let waitingForDrain = false;
@@ -113,10 +135,16 @@ function Stream(s) {
113135
s.addListener('end', abandonQueue);
114136

115137
s.addListener('data', function (d) {
138+
if (dead) return;
139+
const have = self.buf ? self.buf.length : 0;
140+
if (have + d.length > MAX_STREAM_BYTES) {
141+
rejectLimit();
142+
return;
143+
}
116144
if (self.buf) {
117-
const b = buffer.Buffer.allocUnsafe(self.buf.length + d.length);
118-
self.buf.copy(b, 0, 0, self.buf.length);
119-
d.copy(b, self.buf.length, 0, d.length);
145+
const b = buffer.Buffer.allocUnsafe(have + d.length);
146+
self.buf.copy(b, 0, 0, have);
147+
d.copy(b, have, 0, d.length);
120148
self.buf = b;
121149
} else {
122150
self.buf = d;
@@ -152,10 +180,24 @@ function Stream(s) {
152180
self.emit('msg', msg);
153181
}
154182
});
183+
184+
s.addListener('close', dropBuf);
185+
s.addListener('end', dropBuf);
186+
/* Any 'error' listener counts as handling in Node, so this must re-emit
187+
* on Stream. Skip when already dead: rejectLimit emits then destroy(err),
188+
* which fires this listener again. */
189+
s.addListener('error', function (err) {
190+
dropBuf();
191+
if (!dead) {
192+
self.emit('error', err);
193+
}
194+
});
155195
}
156196

157197
util.inherits(Stream, events.EventEmitter);
158198

159199
exports.pack = pack;
160200
exports.unpack = unpack;
161201
exports.Stream = Stream;
202+
exports.MAX_STREAM_BYTES = MAX_STREAM_BYTES;
203+
exports.MAX_STDIN_BYTES = MAX_STDIN_BYTES;

‎package-lock.json‎

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎package.json‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,10 @@
2828
"lib": "lib"
2929
},
3030
"engines": {
31-
"node": ">=18"
31+
"node": "^22"
3232
},
3333
"dependencies": {
34-
"nan": "^2.23.1"
34+
"nan": "2.28.0"
3535
},
3636
"scripts": {
3737
"test": "node --test test/bigint.test.js test/cli.test.js test/coverage-native.test.js test/lazy.test.js test/msgpack.test.js test/pack-hints.test.js test/regression.test.js test/security.test.js test/worker.test.js",
@@ -48,6 +48,6 @@
4848
"gypfile": true,
4949
"license": "BSD-3-Clause",
5050
"devDependencies": {
51-
"c8": "^12.0.0"
51+
"c8": "12.0.0"
5252
}
5353
}

0 commit comments

Comments
 (0)