[libcxx-commits] [PATCH] D91172: [17/N] [libcxx] Implement the read_symlink function for windows

[Adrian McCarthy via Phabricator via libcxx-commits]

amccarth added a comment.

This looks like it'll work just fine, but I've added some inline questions for readability and robustness.



================
Comment at: libcxx/src/filesystem/operations.cpp:1495
+#if defined(_LIBCPP_WIN32API)
+  uint8_t buf[MAXIMUM_REPARSE_DATA_BUFFER_SIZE + sizeof(wchar_t)];
+  detail::WinHandle h(p.c_str(), FILE_READ_ATTRIBUTES,
----------------
What's the purpose of the `+ sizeof(wchar_t)` here?


================
Comment at: libcxx/src/filesystem/operations.cpp:1505
+  LIBCPP_REPARSE_DATA_BUFFER *reparse =
+      reinterpret_cast<LIBCPP_REPARSE_DATA_BUFFER *>(buf);
+  if (reparse->ReparseTag != IO_REPARSE_TAG_SYMLINK)
----------------
 This seems like a reasonable time to use `const auto *`.



================
Comment at: libcxx/src/filesystem/operations.cpp:1507
+  if (reparse->ReparseTag != IO_REPARSE_TAG_SYMLINK)
+    return err.report(make_error_code(errc::invalid_argument));
+  auto &symlink = reparse->SymbolicLinkReparseBuffer;
----------------
I'm wondering whether it's worth doing some bounds checking on `out` and the offsets and lengths into `PathBuffer`.  Is the data from DeviceIoControl trustworthy?  Could somebody create a bad reparse point that would cause this code to access arbitrary data?

I think the answer is probably not.

Does libcxx have a "debug" version with assertions enabled?


================
Comment at: libcxx/src/filesystem/operations.cpp:1508
+    return err.report(make_error_code(errc::invalid_argument));
+  auto &symlink = reparse->SymbolicLinkReparseBuffer;
+  if (symlink.PrintNameLength == 0) {
----------------
`const auto &`?


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

https://reviews.llvm.org/D91172