test: move test-exec.js out of pummel by Trott · Pull Request #25686 · nodejs/node

Trott · 2019-01-24T20:28:53Z

Move test/pummel/test-exec.js to
test/sequential/test-child-process-exec.js. The test does not seem to
use too much resources to be put in sequential. (It does, at least as
written, fail if it has too many other things to compete with, so let's
leave it in sequential for now rather than parallel.)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Move test/pummel/test-exec.js to test/sequential/test-child-process-exec.js. The test does not seem to use too much resources to be put in sequential. (It does, at least as written, fail if it has too many other things to compete with, so let's leave it in sequential for now rather than parallel.)

nodejs-github-bot · 2019-01-24T20:28:55Z

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2374/pipeline

Trott · 2019-01-24T20:29:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/20307/

addaleax

Looks like something that we could also put into parallel if we want?

~~It does, however, fail locally for me on master, most likely because of @Fishrock123’s freshly-landed #24951?~~

Fishrock123 · 2019-01-24T20:47:25Z

@addaleax Make sure you grab latest master, it has a fix.

mhdawson

LGTM

Fishrock123

Do we need to do this? Do we even need this test? What isn't covered by existing parallel/test-child-processes-exec* tests?

Code coverage doesn't seem to indicate anything glaring..

Trott · 2019-01-24T22:02:55Z

Do we need to do this? Do we even need this test? What isn't covered by existing parallel/test-child-processes-exec* tests?

I'd be OK removing it.

Remove redundant test/pummel/test-exec.js. Refs: nodejs#25686 (review)

Remove redundant test/pummel/test-exec.js. Refs: nodejs#25686 (review) PR-URL: nodejs#25722 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott · 2019-01-28T10:07:45Z

Closed in favor of #25722

Remove redundant test/pummel/test-exec.js. Refs: #25686 (review) PR-URL: #25722 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

* feat: 2.0.0 security modernization 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. * fix: address review round 2 on pack/unpack and CLI 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+. * fix: unpack __proto__, pack TryCatch, Stream emit order 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. * fix: pack top-level Buffer as bin, not toJSON map 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> * fix: load native addon in worker_threads 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 #60 * test: lock Stream 0 and Python bin-map unpack 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 #10 Refs #44 * feat: ship TypeScript types for pack, unpack, and Stream 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 #39 * fix: thread_local sbuf pool and NAN_MODULE_WORKER_ENABLED Address review on PR #2 for worker_threads (#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 * test: raise JS and native coverage to 95% 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). * ci: fail coverage job under 95% 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. * docs: document npm run coverage in README Acceptance requires the coverage script in the Building section and a pointer to COVERAGE.md for gates and remainder. * docs: address review — license history, CLI, current benchmarks 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. * fix: restore contributors and cite msgpack-node#25686 Restore the master contributors list in package.json. Point SECURITY.md, src/msgpack.cc, and the leak regression at #25686 instead of nodejs/node#25686. Correct the Stream comment: bytesRemaining is thread_local. Add windows-latest to the CI matrix. Closes #25686 * fix: Windows CI native rebuild and Stream test comment 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. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Jan 24, 2019

addaleax approved these changes Jan 24, 2019

View reviewed changes

addaleax added the child_process Issues and PRs related to the child_process subsystem. label Jan 24, 2019

mhdawson approved these changes Jan 24, 2019

View reviewed changes

Fishrock123 suggested changes Jan 24, 2019

View reviewed changes

Trott added a commit to Trott/io.js that referenced this pull request Jan 26, 2019

test: remove pummel/test-exec

f199be1

Remove redundant test/pummel/test-exec.js. Refs: nodejs#25686 (review)

Trott mentioned this pull request Jan 26, 2019

test: remove pummel/test-exec #25722

Closed

2 tasks done

Trott closed this Jan 28, 2019

Trott deleted the de-pummel-exec branch January 13, 2022 22:51

godsflaw mentioned this pull request Sep 10, 2026

feat: 2.0.0 security modernization msgpack/msgpack-node#25687

Merged

3 tasks done

Navigation Menu

Uh oh!

test: move test-exec.js out of pummel - #25686

test: move test-exec.js out of pummel#25686
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:de-pummel-exec

Trott commented Jan 24, 2019

Uh oh!

nodejs-github-bot commented Jan 24, 2019

Uh oh!

Trott commented Jan 24, 2019

Uh oh!

addaleax left a comment •

edited

Loading

Uh oh!

Fishrock123 commented Jan 24, 2019

Uh oh!

mhdawson left a comment

Uh oh!

Fishrock123 left a comment

Uh oh!

Trott commented Jan 24, 2019

Uh oh!

Trott commented Jan 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

Trott commented Jan 24, 2019

Checklist

Uh oh!

nodejs-github-bot commented Jan 24, 2019

Uh oh!

Trott commented Jan 24, 2019

Uh oh!

addaleax left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jan 24, 2019

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Jan 24, 2019

Uh oh!

Trott commented Jan 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

addaleax left a comment •

edited

Loading