Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: do not read string out of bounds #51358

Merged
merged 1 commit into from
Jan 6, 2024

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Jan 4, 2024

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 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
Copy link
Member

anonrig commented Jan 4, 2024

Can you add a test?

@zcbenz
Copy link
Contributor Author

zcbenz commented Jan 4, 2024

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 kvakil self-requested a review January 4, 2024 07:11
@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2024
@github-actions 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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@zcbenz
Copy link
Contributor Author

zcbenz commented Jan 4, 2024

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 kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2024
@github-actions 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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@kvakil kvakil added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 6, 2024
@nodejs-github-bot 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 nodejs-github-bot merged commit 46bc0ff into nodejs:main Jan 6, 2024
53 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 46bc0ff

marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Jan 12, 2024
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
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
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 marco-ippolito mentioned this pull request Mar 1, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
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 pushed a commit that referenced this pull request Mar 25, 2024
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 richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants