[Support][Windows] Fix incorrect GetFinalPathNameByHandleW() return value check in realPathFromHandle()

[ArcsinX]

Summary

GetFinalPathNameByHandleW(,,N,) returns:

• < N on success (this value does not include the size of the terminating null character)
• >= N if buffer is too small (this value includes the size of the terminating null character)

So, when N == Buffer.capacity() - 1, we need to resize buffer if return value is > Buffer.capacity() - 2.
Also, we can set N to Buffer.capacity().

Thus, without this patch realPathFromHandle() returns unfilled buffer when length of the final path of the file is equal to Buffer.capacity() or Buffer.capacity() - 1.

[andrewng]

llvm/lib/Support/Windows/Path.inc

355

If Buffer.capacity() == 0 this check will fail. This case would have also caused issues with the previous code.

[ArcsinX]

Handle Buffer.capacity() == 0 case correcly.

[ArcsinX]

llvm/lib/Support/Windows/Path.inc

355

Thanks. Fixed.

[andrewng]

llvm/lib/Support/Windows/Path.inc

355

Perhaps this would be better still?

[ArcsinX]

Check GetFinalPathNameByHandleW() return value for zero first

[ArcsinX]

Check GetFinalPathNameByHandleW() return value for non-zero first

[ArcsinX]

llvm/lib/Support/Windows/Path.inc

355

Yes, I think it's reasonable. Thanks, fixed.

[andrewng]

llvm/lib/Support/Windows/Path.inc

355–356

I did consider this change too, but then I decided not to assume that the call to GetFinalPathNameByHandleW below is going to succeed.

[ArcsinX]

llvm/lib/Support/Windows/Path.inc

355–356

Yes, you are right.
Every of GetFinalPathNameByHandleW() calls could return 0 in case of failure, so we need to check CountChars for 0 for both calls.
So, I have reverted that change.

[andrewng]

LGTM with that last one change.

llvm/lib/Support/Windows/Path.inc

360

Sorry, just noticed that this invocation uses Buffer.data() whereas the call above uses Buffer.begin(). Would be good to make it the same. Looks like begin() is the preferred style in this file.

[ArcsinX]

Buffer.data() => Buffer.begin()

[ArcsinX]

Thank you for review.

llvm/lib/Support/Windows/Path.inc

360

Thanks, fixed.

[amccarth]

I know it's not part of this patch, but Phabricator is showing a clang-tidy lint error on the inclusion of <io.h>. While Windows does have one of these, it's only a compatibility hack, and it doesn't seem to be needed. Can you yank that #include <io.h> line before submitting?

[ArcsinX]

Remove #include <io.h>

[ArcsinX]

In D86564#2239262, @amccarth wrote:

I know it's not part of this patch, but Phabricator is showing a clang-tidy lint error on the inclusion of <io.h>. While Windows does have one of these, it's only a compatibility hack, and it doesn't seem to be needed. Can you yank that #include <io.h> line before submitting?

Done

[amccarth]

LGTM

llvm/lib/Support/Windows/Path.inc

 //=== WARNING: Implementation here must contain only generic Windows code that
 //===          is guaranteed to work on *all* Windows variants.
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/WindowsError.h"
 #include <fcntl.h>
-#include <io.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 
 // These two headers must be included last, and make sure shlobj is required
 // after Windows.h to make sure it picks up our definition of _WIN32_WINNT
 #include "llvm/Support/Windows/WindowsSupport.h"
 #include <shellapi.h>
 #include <shlobj.h>
std::error_code is_local(const Twine &path, bool &result) {
   if (std::error_code ec = widenPath(P, WidePath))
     return ec;
   return is_local_internal(WidePath, result);
 }
 
 static std::error_code realPathFromHandle(HANDLE H,
                                           SmallVectorImpl<wchar_t> &Buffer) {
   DWORD CountChars = ::GetFinalPathNameByHandleW(
-      H, Buffer.begin(), Buffer.capacity() - 1, FILE_NAME_NORMALIZED);
-  if (CountChars > Buffer.capacity()) {
+      H, Buffer.begin(), Buffer.capacity(), FILE_NAME_NORMALIZED);
+  if (CountChars && CountChars >= Buffer.capacity()) {
     // The buffer wasn't big enough, try again.  In this case the return value
     // *does* indicate the size of the null terminator.
     Buffer.reserve(CountChars);
     CountChars = ::GetFinalPathNameByHandleW(
-        H, Buffer.data(), Buffer.capacity() - 1, FILE_NAME_NORMALIZED);
+        H, Buffer.begin(), Buffer.capacity(), FILE_NAME_NORMALIZED);
   }
   if (CountChars == 0)
     return mapWindowsError(GetLastError());
   Buffer.set_size(CountChars);
   return std::error_code();
 }
 
 static std::error_code realPathFromHandle(HANDLE H,