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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
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,
----------------
amccarth wrote:
> What's the purpose of the `+ sizeof(wchar_t)` here?
Not entirely sure - I based it on this: https://github.com/microsoft/STL/blob/master/stl/inc/filesystem#L3198

However https://docs.microsoft.com/en-us/windows-hardware/drivers/ifs/fsctl-get-reparse-point says

> For a REPARSE_DATA_BUFFER structure, this value must be at least REPARSE_DATA_BUFFER_HEADER_SIZE, plus the size of the expected user-defined data, and it must be less than or equal to MAXIMUM_REPARSE_DATA_BUFFER_SIZE.

So with that in mind, it actually seems incorrect to have it be any larger than MAXIMUM_REPARSE_DATA_BUFFER_SIZE. So I guess I should change it to that. But that use in MS STL seems curious...

Or is that stated from the perspective of the implementation of FSCTL_GET_REPARSE_POINT, saying that the function may only ever set/use up to MAXIMUM_REPARSE_DATA_BUFFER_SIZE bytes of output space, regardless of how large a buffer is allocated?


================
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)
----------------
amccarth wrote:
>  This seems like a reasonable time to use `const auto *`.
> 
Sure, will change that


================
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;
----------------
amccarth wrote:
> 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?
Good point - even in release mode, such checks should be cheap, and the resulting code isn't too bad either.


================
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) {
----------------
amccarth wrote:
> `const auto &`?
Sure


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

https://reviews.llvm.org/D91172