Skip to content

lib: coerce -0 to +0 in various APIs#63556

Open
panva wants to merge 6 commits into
nodejs:mainfrom
panva:negative-zero-fixes
Open

lib: coerce -0 to +0 in various APIs#63556
panva wants to merge 6 commits into
nodejs:mainfrom
panva:negative-zero-fixes

Conversation

@panva
Copy link
Copy Markdown
Member

@panva panva commented May 25, 2026 •
edited
Loading

Followup to #63531

This is not an exhaustive list of these occurences, just the ones I can remotely imagine being exposed to an untrusted user input through a web-based or CLI interface.

The tests demonstrate the process crashing before the ./lib changes.

I used Codex to find and confirm these before diving in.

@panva panva requested a review from ljharb May 25, 2026 08:46
@panva panva added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label May 25, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 25, 2026

Review requested:

  • @nodejs/crypto
  • @nodejs/net

@panva panva changed the title lib: various coerce -0 to +0 in various APIs lib: coerce -0 to +0 in various APIs May 25, 2026
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 25, 2026
aduh95
aduh95 reviewed May 25, 2026
View reviewed changes
Comment thread lib/internal/fs/watchers.js
Comment on lines 178 to +180
validateUint32(interval, 'interval');
// Coerce -0 to +0.
interval += 0;
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we update validateUint32 to reject -0 instead?

Copy link
Copy Markdown
Member Author

@panva panva May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no particular sympathy for either or.

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having it part of the helper is probably more robust than relying on humans to remember for checking for -0

Copy link
Copy Markdown
Contributor

@TheOneTheOnlyJJ TheOneTheOnlyJJ May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having it part of the helper is probably more robust than relying on humans to remember for checking for -0

Maybe even have it part of all uint conversion helpers?

Copy link
Copy Markdown
Member Author

@panva panva May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can certainly start blanket rejecting -0 from these validators in a semver-major followup if you feel like picking it up. #63531 already went the same route of this PR and is in the commit-queue

@codecov
Copy link
Copy Markdown

codecov Bot commented May 25, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 97.36842% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.32%. Comparing base (cebe424) to head (ae9e27c).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/dns/promises.js 50.00% 2 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #63556   +/-   ##
=======================================
  Coverage   90.32%   90.32%           
=======================================
  Files         730      730           
  Lines      234653   234705   +52     
  Branches    43940    43939    -1     
=======================================
+ Hits       211953   212004   +51     
- Misses      14414    14425   +11     
+ Partials     8286     8276   -10
Files with missing lines Coverage Δ
lib/dns.js 98.65% <100.00%> (+0.01%) ⬆️
lib/internal/blocklist.js 91.39% <100.00%> (+0.05%) ⬆️
lib/internal/crypto/cipher.js 97.98% <100.00%> (+0.03%) ⬆️
lib/internal/crypto/diffiehellman.js 97.77% <100.00%> (+0.01%) ⬆️
lib/internal/crypto/hash.js 99.02% <100.00%> (+0.01%) ⬆️
lib/internal/crypto/hkdf.js 100.00% <100.00%> (+1.15%) ⬆️
lib/internal/crypto/keygen.js 93.49% <100.00%> (+0.47%) ⬆️
lib/internal/crypto/random.js 96.09% <100.00%> (+0.02%) ⬆️
lib/internal/dns/utils.js 99.44% <100.00%> (+<0.01%) ⬆️
lib/internal/fs/utils.js 98.44% <100.00%> (+<0.01%) ⬆️
... and 4 more

... and 45 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@panva panva mentioned this pull request May 26, 2026
Closed
ljharb
ljharb approved these changes May 26, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think fixing the C++ side also is a good idea, but can/should be done separately

@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 26, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2026
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 27, 2026

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

panva added 6 commits May 27, 2026 10:00
@panva
crypto: coerce -0 to +0 before native calls
ac93201 
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
@panva
dns: coerce -0 to +0 in lookup and resolver inputs
30a9263 
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
@panva
fs: coerce -0 to +0 in mode flags and watch intervals
80b1401 
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
@panva
net: coerce -0 to +0 in BlockList prefixes
844056b 
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
@panva
zlib: coerce -0 to +0 for crc32 seeds
ac9d613 
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
@panva
wip: removeme CI debug
ae9e27c
@panva panva force-pushed the negative-zero-fixes branch from bb206e1 to ae9e27c Compare May 27, 2026 08:01
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 27, 2026

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

@panva panva removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments

@ljharb ljharb ljharb approved these changes

+1 more reviewer

@TheOneTheOnlyJJ TheOneTheOnlyJJ TheOneTheOnlyJJ left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@panva @nodejs-github-bot @ljharb @aduh95 @TheOneTheOnlyJJ