fix: close review issues #6-#13 - #25694
Closed
enochgroot wants to merge 17 commits into
Closed
enochgroot wants to merge 17 commits into
enochgroot wants to merge 17 commits into
Conversation
Vendor msgpack-c c-7.0.2, fail-closed unpack limits, and fix the sbuffer leak on pack throw (nodejs/node#25686). Replace nodeunit with node:test, require Node 18+, and run GitHub Actions on 18/20/22.
Dates and toJSON apply at every nesting level. Numeric object keys are packed instead of dropped. Cycle marks use V8 private symbols so a user key named _msgpack_stack is no longer stripped. Integral doubles outside uint64/int64 range (e.g. 1e30) pack as float64. Pack recursion is capped at 512 so 8000-deep input throws instead of SIGSEGV. The vendored C unpacker is built with MSGPACK_EMBED_STACK_SIZE=512 so advertised unpack depth matches the walker. Stream emits error on unpack throw and treats packed nil as a message. The CLI bins run on Node 18+.
DefineOwnProperty on map keys so a wire __proto__ cannot replace
the decoded object's prototype. Property reads during pack go
through Nan::TryCatch so a throwing getter or Proxy ownKeys
raises a catchable error instead of aborting on ToLocalChecked.
Stream snapshots bytes_remaining and advances the buffer before
emit('msg'), so a listener that unpacks or throws cannot desync
or replay a frame.
pack() ran a JS pre-pass that called toJSON() on any top-level object.
Buffer.prototype.toJSON exists, so msgpack.pack(Buffer.from([1,2,3]))
emitted a {type:'Buffer',data:[...]} map instead of msgpack bin, and
unpack() no longer returned a Buffer. Nested Buffers were unaffected
because they went straight to the binding.
The pre-pass is redundant: the native JsToMsgpack checks
node::Buffer::HasInstance before the generic object path, PackObject
applies toJSON at every level, and Dates pack as ISO strings natively.
Drop it and forward arguments to the binding unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NODE_MODULE is not context-aware, so requiring msgpack in the main thread then in a Worker throws "Module did not self-register". Register with NODE_MODULE_CONTEXT_AWARE and keep the sbuffer pool, unpack remainder, and cycle-detection key thread_local so workers do not race the main isolate. Fixes msgpack#60
Packed integer 0 must emit on Stream (msgpack-node#44). A map written by python-msgpack with a bin8 payload must unpack Payload as Buffer (msgpack-node#10). Both already work on 2.0.0; these tests keep them so. Refs msgpack#10 Refs msgpack#44
Adds index.d.ts matching the 2.0.0 runtime: pack(...values) returns Buffer, unpack.bytes_remaining is per-thread, Stream wraps a duplex. Fixes msgpack#39
Address review on PR #2 for worker_threads (msgpack#60): - Mark sbuf_pool thread_local so a worker pack cannot share the pool - Register with NAN_MODULE_WORKER_ENABLED instead of NODE_MODULE_CONTEXT_AWARE - Return non-pooled sbuffers to the pool so it actually fills - Add concurrent pack (workers + main) and sequential pool-reuse tests
…urity/modernize-2.0.0
lib/ and bin/ reach 100% statements/branches/functions/lines under c8; src/msgpack.cc reaches 95.9% lines and 99.5% branches under gcovr. New tests: - test/coverage-native.test.js walks every MessagePack format family from hand-built wire bytes, including the ones pack() never emits (float32, str8/16/32, bin16/32, array32, map16/32, all eight ext forms, negative fixint); a truncation point for every header and payload; the kMaxBytes, kMaxContainer and kMaxDepth rejections; 0xc1; the pack-side type dispatch (Symbol, BigInt, non-finite numbers, integer edges, undefined, zero- and multi-argument pack); Date and toJSON failure modes with mark cleanup; and a worker nesting 600 packs deep to saturate the thread-local sbuffer pool and reach the "pool is full, free it" arm of ~PackBuffer. - test/cli.test.js covers the exit-1 paths of both CLIs. The pack-failure arm of json2msgpack is reachable from real JSON only through nesting deeper than the 512-level pack cap; every other pack error needs a value JSON.parse cannot produce. Infrastructure: - binding.gyp grows an msgpack_coverage variable, default 0. Only when it is set to 1 does the addon compile and link with --coverage -O0 -g, so npm install and node-gyp rebuild stay uninstrumented. - npm run coverage runs coverage:js (c8, gated at 95% on all four metrics) then coverage:native (scripts/coverage-native.js). The native script rebuilds instrumented, runs the suite, gates on gcovr --fail-under-line 95 --fail-under-branch 95 over src/ excluding deps/, and always rebuilds uninstrumented afterwards -- including when the gate fails. src/msgpack.cc gains comments only. GCOVR_EXCL_BR markers, each with its reason inline, mark branches unreachable without stubbing malloc or V8: allocation-failure arms of msgpack_pack_*, empty-MaybeLocal guards, Skip() calls a preceding CheckBytes has already proved safe, and the post-ScanOne error tail of Unpack. No production code was removed. COVERAGE.md lists all 45 marked lines, the 20 still-uncovered lines, the 2 still-uncovered branches and the un-gated numbers (70.6% raw, 86.3% throw-excluded, 99.5% as shipped).
Adds an ubuntu-latest / Node 20 job that installs gcovr via pip when it is not already present and runs npm run coverage. Both halves are threshold- gated, so the job fails when JS or native coverage drops below 95%. The existing node 18/20/22 x ubuntu/macos test matrix is unchanged.
Acceptance requires the coverage script in the Building section and a pointer to COVERAGE.md for gates and remainder.
Strip personal-fork branding so this reads as the upstream 2.0.0 tree. Document that vendored msgpack-c c-7.0.2 is Boost Software License 1.0 (relicensed from Apache-2.0 in msgpack-c 1.3.0). Restore Command Line Utilities and Benchmarks, refresh the bench runner for Node 18+, and record current numbers.
Restore the master contributors list in package.json. Point SECURITY.md, src/msgpack.cc, and the leak regression at msgpack#25686 instead of nodejs/node#25686. Correct the Stream comment: bytesRemaining is thread_local. Add windows-latest to the CI matrix. Closes msgpack#25686
Pin the Windows matrix to windows-2022 so node-gyp 10/11 (Node 18/20/22) can find Visual Studio. windows-latest currently ships VS 2026, which those node-gyp versions report as unknown version "undefined". Run install and test as one bash command so a failed rebuild cannot continue into npm test. List test files explicitly so npm test works on Windows Node 18/20 (cmd.exe does not expand *, and node --test globs need Node 21+). The Stream test comment now matches thread_local bytesRemaining.
Pin nan 2.28.0 and c8 12.0.0; CI uses npm ci. Raise engines.node to >=22 and drop Node 18/20 from CI. Pin GitHub Actions to full SHAs. Cap Stream receive before allocate and drop buf on close. Apply kMaxContainer to pack array/map walks. Cap CLI stdin concat at MAX_STDIN_BYTES. Document a risk-based bump window in SECURITY.md. Closes #6 #7 #8 #9 #10 #11 #12 #13
Collaborator
Author
|
Closing. This branch still carries the pre-squash 2.0 history, so GitHub is showing 17 commits / 116 files against master. The scoped #6–#13 fix is enochgroot#14 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #6 #7 #8 #9 #10 #11 #12 #13
One PR for the eight OPEN review findings on the 2.0 modernization tree.
Changes
nanto exact2.28.0; lockfile matches; CI usesnpm ci.engines.nodeis>=22; CI matrix is Node 22/24 only (no 18/20).msgpack.Streamrejects a receive concat that would exceedMAX_STREAM_BYTESbefore allocate; dropsbufon close/end/error.uses:pinned to full SHAs (checkout@v4.2.2,setup-node@v4.4.0,setup-python@v5.6.0).pack()snapshotsLength()then applieskMaxContaineron array and map walks.json2msgpack/msgpack2jsonrefuse stdin larger thanMAX_STDIN_BYTESbefore concat.SECURITY.mddocuments a risk-based bump window.c8to exact12.0.0.Tests
npm test: 135 pass.JS coverage gate 95% (c8
--check-coverage).Native coverage: 96.0% lines, 99.5% branches, 100% functions (
gcovr --fail-under-line 95 --fail-under-branch 95).Sabotage: Stream cap tests fail without the allocate check; pack sparse-array test fails without
kMaxContainer; CLI tests fail without the stdin cap.Coverage gate in
package.json/COVERAGE.mdis unchanged.Opened from fork enochgroot/msgpack-node branch fix/review-open-6-13.