[Support] Fix utf16 path's index upper bound

[danix800]

Summary

Size of paths in utf8 is possibly bigger than in utf16.

[andrewng]

The usual practice is to include the entire context in the diff. However, the change itself LGTM.

[danix800]

Full context attached @andrewng .

[danix800]

Could anyone merge this revision if acceptable? I don't have commit access.

[andrewng]

In D83321#2140620, @danix800 wrote:

Full context attached @andrewng .

Thanks for doing that, makes it easier for reviewers to see around the code without having to load up the file in question.

Perhaps this particular use case needs to be added to the unit testing ("llvm/unittests/Support/Path.cpp")?

[danix800]

It's just so obvious.

The unit-test TEST_F(FileSystemTest, widenPath) in llvm/unittests/Support/Path.cpp:2080-2146
has already demonstrated this.

#ifdef _WIN32
TEST_F(FileSystemTest, widenPath) {
  const std::wstring LongPathPrefix(L"\\\\?\\");

  // Test that the length limit is checked against the UTF-16 length and not the
  // UTF-8 length.
  std::string Input("C:\\foldername\\");
  const std::string Pi("\xcf\x80"); // UTF-8 lower case pi.
  // Add Pi up to the MAX_PATH limit.
  const size_t NumChars = MAX_PATH - Input.size() - 1;
  for (size_t i = 0; i < NumChars; ++i)
    Input += Pi;
  // Check that UTF-8 length already exceeds MAX_PATH.
  EXPECT_TRUE(Input.size() > MAX_PATH);
  SmallVector<wchar_t, MAX_PATH + 16> Result;
  ASSERT_NO_ERROR(windows::widenPath(Input, Result));
  // Result should not start with the long path prefix.
  EXPECT_TRUE(std::wmemcmp(Result.data(), LongPathPrefix.c_str(),
                           LongPathPrefix.size()) != 0);
  EXPECT_EQ(Result.size(), (size_t)MAX_PATH - 1);

  // Add another Pi to exceed the MAX_PATH limit.

Input.size() > MAX_PATH is expected to be true and Result.size() is expected to be MAX_PATH - 1, meaning
that PathUTF16[Path.size() - 1] is an out-of-bound array access.

[andrewng]

That test is for widenPath and it does cover a similar situation (I know because I wrote it). The code you have changed relates to directory_iterator_construct which does not have coverage of the situation where the UTF16 length is less than the UTF8 length, i.e. the issue that you are fixing.

If another reviewer is happy to approve this change without any test coverage, then that's fine with me.

llvm/lib/Support/Windows/Path.inc

std::error_code detail::directory_iterator_construct(detail::DirIterState &IT,
                                                      bool FollowSymlinks) {
   SmallVector<wchar_t, 128> PathUTF16;
 
   if (std::error_code EC = widenPath(Path, PathUTF16))
     return EC;
 
   // Convert path to the format that Windows is happy with.
   if (PathUTF16.size() > 0 &&
-      !is_separator(PathUTF16[Path.size() - 1]) &&
-      PathUTF16[Path.size() - 1] != L':') {
+      !is_separator(PathUTF16[PathUTF16.size() - 1]) &&
+      PathUTF16[PathUTF16.size() - 1] != L':') {
     PathUTF16.push_back(L'\\');
     PathUTF16.push_back(L'*');
   } else {
     PathUTF16.push_back(L'*');
   }
 
   //  Get the first directory entry.
   WIN32_FIND_DATAW FirstFind;