src: do not read string out of bounds

[zcbenz]

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.

[anonrig]

Can you add a test?

[zcbenz]

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.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[zcbenz]

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.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 46bc0ff