FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: do not read string out of bounds by zcbenz · Pull Request #51358 · nodejs/node · GitHub

/ node Public

src: do not read string out of bounds - #51358

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
zcbenz:str-out-of-bounds
Jan 6, 2024
ConversationCommits (1)ChecksFiles changed
Merged

src: do not read string out of bounds#51358
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
zcbenz:str-out-of-bounds

Conversation

zcbenz commented Jan 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

I'm seeing an assertion from libc++ when compiling Node with GN build:

../../third_party/gn/third_party/libc++/src/include/string_view:408: assertion __pos < size() failed: string_view[] index out of bounds

Which comes from the new C++ version of NormalizeString from #50758:

  const auto pathLen = path.size();
  for (uint8_t i = 0; i <= pathLen; ++i) {
    if (i < pathLen) {
      code = path[i];
    } else if (IsPathSeparator(path[i])) {
      break;
    } else {
      code = node::kPathSeparator;
    }

There seems to be a few errors:

  1. i should a size_t, otherwise it can only parse strings at most 255 characters.
  2. This code always ends up reading path[path.size()], which reads out-of-bounds.

The origin js function was written in an unusual way which I don't quite understand, so I might be missing something here.

If I'm not missing anything, I suggest a fast track merge since it is a out-of-bounds read bug.

nodejs-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 Jan 4, 2024

anonrig commented Jan 4, 2024

Copy link
Copy Markdown
Member

Can you add a test?

zcbenz commented Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

For out-of-bound reads it is usually detected with memory sanitizer or assertions in C++ standard library, the GN build has all the tools and this was caught when running the tests with GN build of Node. I don't have a good idea how to write a test for this with the toolings in upstream.

kvakil self-requested a review January 4, 2024 07:11
kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2024

nodejs-github-bot commented Jan 4, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jan 4, 2024

Copy link
Copy Markdown
Collaborator

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

zcbenz force-pushed the str-out-of-bounds branch from c778390 to 7bc8675 Compare January 4, 2024 12:20
zcbenz force-pushed the str-out-of-bounds branch from 7bc8675 to ea42eed Compare January 4, 2024 12:20

zcbenz commented Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

I have updated this PR so the C++ code is a correct translation of the JS code. I still don't quite understand what the js version of normalizeString is doing but there should be no risk since they have identical logics now.

kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2024

nodejs-github-bot commented Jan 4, 2024

Copy link
Copy Markdown
Collaborator

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

zcbenz mentioned this pull request Jan 5, 2024
Closed

nodejs-github-bot commented Jan 5, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jan 5, 2024

Copy link
Copy Markdown
Collaborator

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

kvakil added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 6, 2024
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 6, 2024
nodejs-github-bot merged commit 46bc0ff into nodejs:main Jan 6, 2024

nodejs-github-bot commented Jan 6, 2024

Copy link
Copy Markdown
Collaborator

Landed in 46bc0ff

zcbenz mentioned this pull request Jan 9, 2024
Closed
27 tasks
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Jan 12, 2024
src: do not read string out of bounds
21e8f64 
PR-URL: nodejs#51358
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Keyhan Vakil <kvakil@sylph.kvakil.me>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Medhansh404 pushed a commit to Medhansh404/node that referenced this pull request Jan 19, 2024
src: do not read string out of bounds
80c11c1 
PR-URL: nodejs#51358
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Keyhan Vakil <kvakil@sylph.kvakil.me>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
targos pushed a commit that referenced this pull request Feb 15, 2024
src: do not read string out of bounds
36e42aa 
PR-URL: #51358
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Keyhan Vakil <kvakil@sylph.kvakil.me>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
marco-ippolito mentioned this pull request Mar 1, 2024
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
src: do not read string out of bounds
2e65389 
PR-URL: #51358
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Keyhan Vakil <kvakil@sylph.kvakil.me>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
richardlau mentioned this pull request Mar 25, 2024
Merged
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

anonrig anonrig approved these changes
gengjiawen gengjiawen approved these changes
+1 more reviewer
kvakil kvakil approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. 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

Back | FazBrowse Home | New Git URL