Skip to content

buffer: validate end argument of indexOf methods - #66590

Open
gengjiawen wants to merge 1 commit into
nodejs:mainfrom
gengjiawen:buffer-indexof-validate-end
Open

gengjiawen wants to merge 1 commit into
nodejs:mainfrom
gengjiawen:buffer-indexof-validate-end

Conversation

@gengjiawen

Copy link
Copy Markdown
Member

Buffer.prototype.indexOf(), lastIndexOf() and includes() pass any end that is neither a string nor undefined straight to the binding, which CHECKs that it is a number, so buf.indexOf('a', 0, null) aborts the process. Numbers that do not fit in an int64 (Infinity, 1e20) go through an undefined double-to-int64 conversion and the search returns -1 on x64.

This validates end with validateNumber() in bidirectionalIndexOf() and clamps it to [0, buffer.byteLength] before it reaches C++:

  • non-numbers now throw ERR_INVALID_ARG_TYPE instead of aborting
  • Infinity and other values past the end search the whole buffer
  • NaN and -Infinity select an empty range (they already returned -1 before)
  • finite values behave as before, the binding already clamped them to the same range

Fixes: #66589
Refs: #62390

AI disclosure: written with the help of Claude (claude:opus-5.5). I checked the bug against the source and the latest nightly, and reviewed the fix and tests.

Buffer.prototype.indexOf(), lastIndexOf() and includes() passed any
`end` that was neither a string nor undefined straight to the binding,
which CHECKs that it is a number, so values such as `null` or `{}`
aborted the process. Numbers that do not fit in an int64 (`Infinity`,
`1e20`) went through an undefined double-to-int64 conversion and made
the search return -1 on x64.

Throw ERR_INVALID_ARG_TYPE for non-number values and clamp numbers to
[0, buffer.byteLength] before calling into the binding.

Fixes: nodejs#66589
Refs: nodejs#62390
Signed-off-by: Jiawen Geng <technicalcute@gmail.com>
Assisted-by: claude:opus-5.5
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels Oct 8, 2026
@codecov

codecov Bot commented Oct 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.43%. Comparing base (c3189d4) to head (8a5c8ae).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66590      +/-   ##
==========================================
- Coverage   90.45%   90.43%   -0.02%     
==========================================
  Files         791      791              
  Lines      276562   276572      +10     
  Branches    53111    53116       +5     
==========================================
- Hits       250154   250131      -23     
- Misses      16801    16841      +40     
+ Partials     9607     9600       -7     
Files with missing lines Coverage Δ
lib/buffer.js 99.74% <100.00%> (+<0.01%) ⬆️

... and 28 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.

@gengjiawen
gengjiawen requested review from jasnell and ronag October 13, 2026 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Buffer#indexOf()/lastIndexOf()/includes() abort the process when end is not a number

2 participants