<filesystem>::space fails with no read access to root #552

mscottmueller · 2020-02-27T06:45:46Z

The method filesystem::space may not function in a least required privilege environment.

Description

Avoid the unnecessary call to GetVolumePathNameW. Read access on the root of the mounted volume is for this call is required in order for it to succeed. This call is unnecessary as GetDiskFreeSpaceExW operates on a full path and will traverse symlinks and junctions on its own. The appropriate read permissions on the given path will be taken into consideration. It is expected that read permissions would be required for the target path, but not for the root of the mounted volume.

Note that GetVolumePathNameW was required for GetDiskFreeSpace in the past, though this was because the root path of the target volume was expected for the path parameter.

This issue was discovered when comparing the MSVC filesystem implementation against boost::filesystem and observing differences in behavior within LRP environments.

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

Identifiers in product code changes are properly _Ugly as per
https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
The STL builds successfully and all tests have passed (must be manually
verified by an STL maintainer before automated testing is enabled on GitHub,
leave this unchecked for initial submission).
These changes introduce no known ABI breaks (adding members, renaming
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.).
These changes were written from scratch using only this repository,
the C++ Working Draft (including any cited standards), other WG21 papers
(excluding reference implementations outside of proposed standard wording),
and LWG issues as reference material. If they were derived from a project
that's already listed in NOTICE.txt, that's fine, but please mention it.
If they were derived from any other project (including Boost and libc++,
which are not yet listed in NOTICE.txt), you must mention it here,
so we can determine whether the license is compatible and what else needs
to be done.

Avoid the unnecessary call to GetVolumePathNameW because read access for this call is required on the root of the mounted volume. This call used to be required for GetDiskFreeSpace, but should not be used for GetDiskFreeSpaceExW, where the full path should be used. The appropriate read permissions on the given path will be taken into consideration.

BillyONeal

If this API call change is actually correct then the we should do the following:

Delete _Fs_space_attempt
Remove the GetFileAttributesW "exists" check
Move __std_fs_space back to filesystem.cpp, since the problematic function for "app" programs was the GetVolumePathName call we apparently don't need
Delete filesystem_space.cpp.

BillyONeal · 2020-02-27T07:06:47Z

Thanks for your contribution!

BillyONeal · 2020-02-27T17:39:45Z

(And of course the syntax errors need be fixed :) )

stl/src/filesystem_space.cpp

BillyONeal · 2020-02-27T22:28:27Z

I did some testing with this and unfortunately there's a wrinkle both the old implementation and the proposed implementation break on. Consider the situation where the path provided to space is a symbolic link -- the standard requires the function to return information about the symbolic link target, not the symbolic link itself.

I observe that the call to GetVolumePathNameW is nonconforming in this regard -- it doesn't follow the symbolic link.

GetDiskFreeSpaceEx without GetVolumePathNameW requires that the input is a handle to a directory; it cannot be a handle to a file. Therefore we can't just check if GetDiskFreeSpaceEx fails and remove the last path element: the last path element may have been a symbolic link to a completely different drive. In that case we need to do the moral equivalent of canoncial which likely requires even more permissions than GetVolumePathNameW. :(

As a result I'm reluctant to take the approach in this PR as it would cause such cases to start working, then we'd have to break those customers again to repair the symlink issues. I'm investigating alternatives now....

Update filesystem.cpp to remove GetVolumePathNameW and GetFileAttributesW

msftclas · 2020-02-27T23:09:50Z

All CLA requirements met.

mscottmueller · 2020-02-27T23:14:41Z

What would it mean to follow a symlink to its volume and then call space on it? I don't understand that use case, unless the scenario is that we'd be changing the size of the file pointed to by the symlink itself...

BillyONeal · 2020-02-27T23:26:27Z

What would it mean to follow a symlink to its volume and then call space on it? I don't understand that use case, unless the scenario is that we'd be changing the size of the file pointed to by the symlink itself...

Consider D:\some_file.txt, and a symbolic link C:\example\some_file_symlink.txt where that link points to D:\some_file.txt. In this condition, space must return information about D:, not C:.

I think I have some changes staged that will fix this...

BillyONeal · 2020-02-27T23:35:27Z

…er/STL into filesystem_space_LRP # Conflicts: # stl/src/filesystem.cpp

BillyONeal · 2020-02-27T23:41:34Z

@mscottmueller Can you sign our CLA and check that you're happy with my changes?

Thanks!

mscottmueller · 2020-02-28T00:46:03Z

Thank you so much for reviewing/fixing this issue, @BillyONeal !

StephanTLavavej · 2020-02-29T00:54:21Z

stl/src/filesystem.cpp

+        return __std_win_error::_Success;
+    }
+
+    auto _Last_error         = __std_win_error{GetLastError()};


This form of initialization is inconsistent with our usual practices. (We don't say auto and then repeat the type on the right, as it is needlessly verbose, and doesn't really guard against uninitialized variables. We use auto when the RHS is a cast etc.)

Can this be __std_win_error _Last_error{GetLastError()};? That would be consistent with usage elsewhere (e.g. in __std_fs_directory_iterator_open).

stl/src/filesystem.cpp

StephanTLavavej · 2020-02-29T01:00:15Z

stl/src/filesystem.cpp

+            }
+
+            _Actual_length = __vcrt_GetFinalPathNameByHandleW(
+                _Handle._Get(), _Buf.get() + 14, _Buf_count - 14, FILE_NAME_NORMALIZED | VOLUME_NAME_NT);


The magic number 14 is repeated throughout this function. I strongly believe that it should be centralized with a comment. (It appears to be the number of characters in \\?\GLOBALROOT but I don't know for sure.)

…lization for _Last_error.

StephanTLavavej

Ok to merge after addressing the static constexpr array.

stl/src/filesystem.cpp

BillyONeal · 2020-03-03T17:40:20Z

Thanks for your contribution! This will ship in Visual Studio 2019 version 16.7 preview 1.

mscottmueller requested a review from a team as a code owner February 27, 2020 06:45

mscottmueller mentioned this pull request Feb 27, 2020

<filesystem>: space(R"(path\to\some\directory)") requires read access at the root level #549

Closed

BillyONeal suggested changes Feb 27, 2020

View reviewed changes

CaseyCarter reviewed Feb 27, 2020

View reviewed changes

stl/src/filesystem_space.cpp Outdated Show resolved Hide resolved

BillyONeal self-assigned this Feb 27, 2020

StephanTLavavej added the bug Something isn't working label Feb 27, 2020

StephanTLavavej linked an issue Feb 27, 2020 that may be closed by this pull request

<filesystem>: space(R"(path\to\some\directory)") requires read access at the root level #549

Closed

Revert commit 18063ce and address possibility that the target is a file

f7b2e85

Update filesystem.cpp to remove GetVolumePathNameW and GetFileAttributesW

BillyONeal added 2 commits February 27, 2020 15:37

Merge filesystem_space.cpp, add symlinks test cases and fix failures.

c9bf2f3

Merge branch 'filesystem_space_LRP' of https://github.com/mscottmuell…

b401f36

…er/STL into filesystem_space_LRP # Conflicts: # stl/src/filesystem.cpp

BillyONeal approved these changes Feb 27, 2020

View reviewed changes

BillyONeal added 2 commits February 28, 2020 16:22

Merge remote-tracking branch 'origin/master' into filesystem_space_LRP

5d3e9b4

Add missing out of memory test.

c2aa14c

StephanTLavavej requested changes Feb 29, 2020

View reviewed changes

BillyONeal added 2 commits February 28, 2020 19:43

Merge remote-tracking branch 'origin/master' into filesystem_space_LRP

2da8c02

Name _Dos_to_nt_prefix, name the "14" constant, and use direct initia…

1bc6cdd

…lization for _Last_error.

BillyONeal requested a review from StephanTLavavej February 29, 2020 04:01

CaseyCarter approved these changes Mar 2, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into filesystem_space_LRP

7111bf3

StephanTLavavej approved these changes Mar 3, 2020

View reviewed changes

stl/src/filesystem.cpp Outdated Show resolved Hide resolved

stl/src/filesystem.cpp Show resolved Hide resolved

stl/src/filesystem.cpp Show resolved Hide resolved

Add static to constexpr array.

1b3f310

StephanTLavavej approved these changes Mar 3, 2020

View reviewed changes

BillyONeal merged commit 922eec2 into microsoft:master Mar 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

<filesystem>::space fails with no read access to root #552

<filesystem>::space fails with no read access to root #552

mscottmueller commented Feb 27, 2020 •

edited by BillyONeal

Loading

BillyONeal left a comment

BillyONeal commented Feb 27, 2020

BillyONeal commented Feb 27, 2020

BillyONeal commented Feb 27, 2020

msftclas commented Feb 27, 2020 •

edited

Loading

mscottmueller commented Feb 27, 2020

BillyONeal commented Feb 27, 2020

BillyONeal commented Feb 27, 2020

BillyONeal commented Feb 27, 2020

mscottmueller commented Feb 28, 2020

StephanTLavavej Feb 29, 2020

StephanTLavavej Feb 29, 2020

StephanTLavavej left a comment

BillyONeal commented Mar 3, 2020

<filesystem>::space fails with no read access to root #552

<filesystem>::space fails with no read access to root #552

Conversation

mscottmueller commented Feb 27, 2020 • edited by BillyONeal Loading

Description

Checklist

BillyONeal left a comment

Choose a reason for hiding this comment

BillyONeal commented Feb 27, 2020

BillyONeal commented Feb 27, 2020

BillyONeal commented Feb 27, 2020

msftclas commented Feb 27, 2020 • edited Loading

mscottmueller commented Feb 27, 2020

BillyONeal commented Feb 27, 2020

BillyONeal commented Feb 27, 2020

BillyONeal commented Feb 27, 2020

mscottmueller commented Feb 28, 2020

StephanTLavavej Feb 29, 2020

Choose a reason for hiding this comment

StephanTLavavej Feb 29, 2020

Choose a reason for hiding this comment

StephanTLavavej left a comment

Choose a reason for hiding this comment

BillyONeal commented Mar 3, 2020

mscottmueller commented Feb 27, 2020 •

edited by BillyONeal

Loading

msftclas commented Feb 27, 2020 •

edited

Loading