Skip to content

buffer: fix end parameter bugs in indexOf/lastIndexOf - #62711

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
ronag:buffer-index-of-end2
Apr 24, 2026
Merged

buffer: fix end parameter bugs in indexOf/lastIndexOf#62711
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
ronag:buffer-index-of-end2

Conversation

@ronag

@ronagronag commented Apr 12, 2026

Copy link
Copy Markdown
Member
  • Fix FastIndexOfNumber parameter order mismatch (end_i64 and is_forward were swapped vs the JS call site and slow path)
  • Clamp negative end values to 0 to prevent size_t overflow in IndexOfString, IndexOfBuffer, and IndexOfNumberImpl
  • Clamp empty needle result to search_end

Fixes: #62873

@ronag
ronag requested review from jasnell and mcollinaApril 12, 2026 20:18
@ronagronag added the buffer Issues and PRs related to the buffer subsystem. label Apr 12, 2026
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Apr 12, 2026
@ronagronag added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2026
@ronag
ronagforce-pushed the buffer-index-of-end2 branch 2 times, most recently from 81afd27 to 927c25aCompareApril 12, 2026 20:28
@codecov

codecovBot commented Apr 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.62%. Comparing base (4563cb3) to head (1d34d6b).
⚠️ Report is 148 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #62711 +/- ##
==========================================
+ Coverage 88.93% 89.62% +0.68% 
==========================================
Files 699 706 +7 Lines 216379 219221 +2842 Branches 41281 42003 +722 ==========================================
+ Hits 192434 196471 +4037 + Misses 15976 14674 -1302 - Partials 7969 8076 +107 
Files with missing linesCoverage Δ
src/node_buffer.cc68.66% <100.00%> (+0.63%)⬆️

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

@ronagronag mentioned this pull request Apr 21, 2026
@ronag
ronag requested a review from ChALkeRApril 21, 2026 15:24
@ronag
ronagforce-pushed the buffer-index-of-end2 branch from 927c25a to 7d46b35CompareApril 21, 2026 23:47
- Fix FastIndexOfNumber parameter order mismatch (end_i64 and
is_forward were swapped vs the JS call site and slow path)
- Clamp negative end values to 0 to prevent size_t overflow in
IndexOfString, IndexOfBuffer, and IndexOfNumberImpl
- Clamp empty needle result to search_end
Signed-off-by: Robert Nagy <ronagy@icloud.com>
Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ronag
ronagforce-pushed the buffer-index-of-end2 branch from 7d46b35 to e4bbd84CompareApril 22, 2026 11:05
@ronagronag added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 24, 2026
@ronag

Copy link
Copy Markdown
MemberAuthor

This is an important bug fix which is unable to get reviews. @nodejs/tsc

@panva

Copy link
Copy Markdown
Member

I don't think this has a place on the TSC agenda. I'm happy to review but at this point this can't land anyway on the account of having formatting issues.

@panvapanva removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 24, 2026
Comment threadsrc/node_buffer.cc Outdated
Comment threadsrc/node_buffer.cc Outdated
Co-authored-by: Robert Nagy <ronagy@icloud.com>
@ronag

Copy link
Copy Markdown
MemberAuthor

I don't think this has a place on the TSC agenda. I'm happy to review but at this point this can't land anyway on the account of having formatting issues.

I guess it's gray zone. Being unable to land an important (security related) fix falls IMHO under the TSC.

Formatting issues resolve. PTAL

@panva

panva commented Apr 24, 2026

Copy link
Copy Markdown
Member

I guess it's gray zone. Being unable to land an important (security related) fix falls IMHO under the TSC.

But there's no rush immediate urgency since it's not due to land on any release given the labels on #62390 yet, or am I reading this wrong. Quite possible.

@panvapanva left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@panvapanva added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 24, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 24, 2026
@github-actionsgithub-actionsBot added the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 24, 2026
@github-actions

Copy link
Copy Markdown
Contributor
Failed to start CI
[SyntaxError: Unexpected token '<', "https://github.com/nodejs/node/actions/runs/24897465125

@panvapanva removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 24, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

nodejs-github-bot commented Apr 24, 2026

Copy link
Copy Markdown
Collaborator

@ronagronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 24, 2026
@nodejs-github-botnodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 24, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/62711
✔ Done loading data for nodejs/node/pull/62711
----------------------------------- PR info ------------------------------------
Title buffer: fix end parameter bugs in indexOf/lastIndexOf (#62711)
Author Robert Nagy <ronagy@icloud.com> (@ronag)
Branch ronag:buffer-index-of-end2 -> nodejs:main
Labels buffer, c++, author ready, needs-ci
Commits 2
- buffer: fix end parameter bugs in indexOf/lastIndexOf
- Apply suggestions from code review
Committers 2
- Robert Nagy <ronagy@icloud.com>
- GitHub <noreply@github.com>
PR-URL: https://github.com/nodejs/node/pull/62711
Fixes: https://github.com/nodejs/node/issues/62873
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/62711
Fixes: https://github.com/nodejs/node/issues/62873
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
--------------------------------------------------------------------------------
ℹ This PR was created on Sun, 12 Apr 2026 20:18:11 GMT
✔ Approvals: 1
✔ - Filip Skokan (@panva) (TSC): https://github.com/nodejs/node/pull/62711#pullrequestreview-4171398766
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2026-04-24T17:16:45Z: https://ci.nodejs.org/job/node-test-pull-request/72904/
- Querying data for job/node-test-pull-request/72904/
✔ Build data downloaded
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ No git cherry-pick in progress
✔ No git am in progress
✔ No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
* branch main -> FETCH_HEAD
✔ origin/main is now up-to-date
- Downloading patch for 62711
From https://github.com/nodejs/node
* branch refs/pull/62711/merge -> FETCH_HEAD
✔ Fetched commits as 9c4ca0a12d1c..1d34d6bdb97c
--------------------------------------------------------------------------------
[main cd8d9d00c1] buffer: fix end parameter bugs in indexOf/lastIndexOf
Author: Robert Nagy <ronagy@icloud.com>
Date: Sun Apr 12 22:16:34 2026 +0200
2 files changed, 52 insertions(+), 11 deletions(-)
[main 1d3743e568] Apply suggestions from code review
Author: Robert Nagy <ronagy@icloud.com>
Date: Fri Apr 24 16:25:08 2026 +0200
1 file changed, 2 insertions(+), 4 deletions(-)
✔ Patches applied
There are 2 commits in the PR. Attempting autorebase.
(node:339) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated.
(Use `node --trace-deprecation ...` to show where the warning was created)
Rebasing (2/4)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
buffer: fix end parameter bugs in indexOf/lastIndexOf
  • Fix FastIndexOfNumber parameter order mismatch (end_i64 and
    is_forward were swapped vs the JS call site and slow path)
  • Clamp negative end values to 0 to prevent size_t overflow in
    IndexOfString, IndexOfBuffer, and IndexOfNumberImpl
  • Clamp empty needle result to search_end

Signed-off-by: Robert Nagy <ronagy@icloud.com>
Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR-URL: #62711
Fixes: #62873
Reviewed-By: Filip Skokan <panva.ip@gmail.com>

[detached HEAD 524e56e6a2] buffer: fix end parameter bugs in indexOf/lastIndexOf
Author: Robert Nagy <ronagy@icloud.com>
Date: Sun Apr 12 22:16:34 2026 +0200
2 files changed, 52 insertions(+), 11 deletions(-)
Rebasing (3/4)
Rebasing (4/4)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Apply suggestions from code review

Co-authored-by: Robert Nagy <ronagy@icloud.com>
PR-URL: #62711
Fixes: #62873
Reviewed-By: Filip Skokan <panva.ip@gmail.com>

[detached HEAD c84de63c63] Apply suggestions from code review
Author: Robert Nagy <ronagy@icloud.com>
Date: Fri Apr 24 16:25:08 2026 +0200
1 file changed, 2 insertions(+), 4 deletions(-)
Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/24906439861

@panvapanva added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 24, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 24, 2026
@nodejs-github-bot
nodejs-github-bot merged commit 8176c2c into nodejs:mainApr 24, 2026
79 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 8176c2c

aduh95 pushed a commit that referenced this pull request May 5, 2026
- Fix FastIndexOfNumber parameter order mismatch (end_i64 and
is_forward were swapped vs the JS call site and slow path)
- Clamp negative end values to 0 to prevent size_t overflow in
IndexOfString, IndexOfBuffer, and IndexOfNumberImpl
- Clamp empty needle result to search_end
Signed-off-by: Robert Nagy <ronagy@icloud.com>
Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR-URL: #62711Fixes: #62873
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@aduh95aduh95 mentioned this pull request May 5, 2026
sxa pushed a commit to sxa/node that referenced this pull request Jun 22, 2026
- Fix FastIndexOfNumber parameter order mismatch (end_i64 and
is_forward were swapped vs the JS call site and slow path)
- Clamp negative end values to 0 to prevent size_t overflow in
IndexOfString, IndexOfBuffer, and IndexOfNumberImpl
- Clamp empty needle result to search_end
Signed-off-by: Robert Nagy <ronagy@icloud.com>
Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR-URL: nodejs#62711Fixes: nodejs#62873
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Signed-off-by: Stewart X Addison <sxa@ibm.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.bufferIssues and PRs related to the buffer subsystem.c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OOB read in Buffer.prototype.indexOf

4 participants

@ronag@panva@nodejs-github-bot@anonrig