[libcxx-commits] [PATCH] D91170: [15/N] [libcxx] Implement the canonical function for windows

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/src/filesystem/operations.cpp:863
+  if (!GetFinalPathNameByHandleW(h, ret, MAX_PATH,
+                                 FILE_NAME_NORMALIZED | VOLUME_NAME_DOS))
+    return err.report(detail::make_windows_error(GetLastError()));
----------------
amccarth wrote:
> This return value needs to be checked more carefully.
> 
> If the final path is longer than the buffer, then `GetFinalPathNameByHandleW` will return the actual length needed, which will convince this if statement that the call succeeded when it didn't, and then we'll process the uninitialized buffer.
> 
> How can the final path require a buffer longer than `MAX_PATH`?  Two ways:
> 
> 1.  The function always returns the `\\?\` prefix, so a path that (with it's terminating `L'\0'`) is exactly `MAX_PATH` will require `MAX_PATH+4` when the prefix is added.
> 
> 2.  `MAX_PATH` applies to most Windows APIs, but not to the actual filesystem.  It's possible to have a final path that's longer than `MAX_PATH` (especially since it's using the `\\?\` prefix).  If we don't think it's worth handling those, fine, but we'd need to detect that case so that we can return an error.  (A common way this happens is when the user uses `subst` to replace a long prefix with a fictitious drive letter.  This is, in fact, how we get certain LLDB source paths under the `MAX_PATH` limit on some of the Windows build bots.)
Thanks, good catch. Will update with a stack buffer that should fit all practical cases (MAX_PATH+10) and fall back on dynamic allocation if that wasn't big enough.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91170/new/

https://reviews.llvm.org/D91170