Page MenuHomePhabricator

[17/N] [libcxx] Implement the read_symlink function for windows
ClosedPublic

Authored by mstorsjo on Nov 10 2020, 8:40 AM.

Details

Diff Detail

Event Timeline

mstorsjo created this revision.Nov 10 2020, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2020, 8:40 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
compnerd added inline comments.
libcxx/src/filesystem/operations.cpp
1233

You should use backup semantics here to ensure that directories can be read.

FWIW, after this patch, it should compile successfully for windows, the later patches only fix remaining behaviours.

libcxx/src/filesystem/operations.cpp
1233

This does use backup semantics; see https://reviews.llvm.org/D91141 for the implementation of the WinHandle helper. The flag FILE_FLAG_BACKUP_SEMANTICS is always included, to keep the calling code terse here.

mstorsjo updated this revision to Diff 305085.Nov 13 2020, 3:47 AM
mstorsjo added a reviewer: amccarth.

Rebased on top of updated patches.

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

libcxx/src/filesystem/operations.cpp
1231

What's the purpose of the + sizeof(wchar_t) here?

1241

This seems like a reasonable time to use const auto *.

1243

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?

1244

const auto &?

mstorsjo added inline comments.Dec 7 2020, 3:08 AM
libcxx/src/filesystem/operations.cpp
1231

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?

1241

Sure, will change that

1243

Good point - even in release mode, such checks should be cheap, and the resulting code isn't too bad either.

1244

Sure

mstorsjo updated this revision to Diff 309855.Dec 7 2020, 3:11 AM
mstorsjo marked 4 inline comments as done.
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Updated according to suggestions.

amccarth accepted this revision.Dec 11 2020, 2:36 PM

LGTM.

libcxx/src/filesystem/operations.cpp
1231

Yes, I think MAXIMUM_REPARSE_DATA_BUFFER_SIZE is the most it'll ever write to the buffer, no matter how large you say the buffer is. I think the corresponding FSCTL_SET_REPARSE_POINT will actually fail if you try to send too much data.

My guess is that the + sizeof(wchar_t) was so that a zero-terminator could be added should anyone every inadvertently treat the buffer as a string.

So I guess this all works, but I wisth

mstorsjo updated this revision to Diff 317584.Jan 19 2021, 8:29 AM

Made a readlink helper in posix_compat.h.

ldionne set the repository for this revision to rG LLVM Github Monorepo.Jan 28 2021, 3:19 PM
mstorsjo removed rG LLVM Github Monorepo as the repository for this revision.Jan 29 2021, 5:00 AM
mstorsjo updated this revision to Diff 320107.Jan 29 2021, 5:03 AM
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Rebased, retriggering CI.

Avoid using PATH_MAX (which exists in mingw but not in MSVC) and use MAXIMUM_REPARSE_DATA_BUFFER_SIZE as fixed limit for the statically allocated buffer that is passed to the readlink() function.

(When this was implemented directly in __read_symlink() before, the windows codepath didn't need to concern itself with other fixed limits but just do its own thing and return a std::filesystem::path, but using the same readlink() interface now to keep the implementation out of operations.cpp.)

mstorsjo updated this revision to Diff 320894.Feb 2 2021, 1:10 PM
mstorsjo removed rG LLVM Github Monorepo as the repository for this revision.
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Rebased, added detail::SSizeT instead of ::ssize_t (for the readlink return value).

ldionne accepted this revision.Feb 2 2021, 1:24 PM
ldionne added a subscriber: ldionne.

LGTM pending CI.

libcxx/src/filesystem/posix_compat.h
44

Just saying, but it looks like those SDKs are pretty terrible to program against for lack of uniformity.

This revision is now accepted and ready to land.Feb 2 2021, 1:24 PM
This revision was landed with ongoing or failed builds.Feb 2 2021, 11:28 PM
This revision was automatically updated to reflect the committed changes.