From 922eec2a1263a040b300656b776d007bc52ed8ba Mon Sep 17 00:00:00 2001 From: Michael Scott Mueller Date: Tue, 3 Mar 2020 09:37:04 -0800 Subject: [PATCH] ::space fails with no read access to root (#552) * ::space fails with no read access to root 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. Co-authored-by: Billy O'Neal --- stl/CMakeLists.txt | 1 - .../stl_base/stl.files.settings.targets | 1 - stl/src/filesystem.cpp | 77 +++++++++++++++++++ stl/src/filesystem_space.cpp | 73 ------------------ tests/std/tests/P0218R1_filesystem/test.cpp | 53 ++++++++++++- 5 files changed, 127 insertions(+), 78 deletions(-) delete mode 100644 stl/src/filesystem_space.cpp diff --git a/stl/CMakeLists.txt b/stl/CMakeLists.txt index 3a771a1a3..5bd82bd33 100644 --- a/stl/CMakeLists.txt +++ b/stl/CMakeLists.txt @@ -238,7 +238,6 @@ endforeach() # Objs that exist in both libcpmt[d][01].lib and msvcprt[d].lib. set(IMPLIB_SOURCES ${CMAKE_CURRENT_LIST_DIR}/src/filesystem.cpp - ${CMAKE_CURRENT_LIST_DIR}/src/filesystem_space.cpp ${CMAKE_CURRENT_LIST_DIR}/src/locale0_implib.cpp ${CMAKE_CURRENT_LIST_DIR}/src/nothrow.cpp ${CMAKE_CURRENT_LIST_DIR}/src/parallel_algorithms.cpp diff --git a/stl/msbuild/stl_base/stl.files.settings.targets b/stl/msbuild/stl_base/stl.files.settings.targets index 8a2158bd9..c4126d80a 100644 --- a/stl/msbuild/stl_base/stl.files.settings.targets +++ b/stl/msbuild/stl_base/stl.files.settings.targets @@ -168,7 +168,6 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception (controlled by IncludeInLink and IncludeInImportLib). --> "); + const auto _Available_c = reinterpret_cast(_Available); + const auto _Total_bytes_c = reinterpret_cast(_Total_bytes); + const auto _Free_bytes_c = reinterpret_cast(_Free_bytes); + if (GetDiskFreeSpaceExW(_Target, _Available_c, _Total_bytes_c, _Free_bytes_c)) { + return __std_win_error::_Success; + } + + __std_win_error _Last_error{GetLastError()}; + _Available_c->QuadPart = ~0ull; + _Total_bytes_c->QuadPart = ~0ull; + _Free_bytes_c->QuadPart = ~0ull; + if (_Last_error != __std_win_error::_Directory_name_is_invalid) { + return _Last_error; + } + + // Input could have been a file; canonicalize and remove the last component. + // We use VOLUME_NAME_NT because it always has a mapping available and we don't care about the canonical path + // being "ugly" due to needing the _Dos_to_nt_prefix. + static constexpr wchar_t _Dos_to_nt_prefix[] = LR"(\\?\GLOBALROOT)"; + constexpr size_t _Dos_to_nt_prefix_count = sizeof(_Dos_to_nt_prefix) / sizeof(wchar_t) - 1; + __crt_unique_heap_ptr _Buf; + DWORD _Actual_length; + { + const _STD _Fs_file _Handle( + _Target, __std_access_rights::_File_read_attributes, __std_fs_file_flags::_Backup_semantics, &_Last_error); + if (_Last_error != __std_win_error::_Success) { + return _Last_error; + } + + DWORD _Buf_count = MAX_PATH; + for (;;) { + _Buf = _malloc_crt_t(wchar_t, _Buf_count); + if (!_Buf) { + return __std_win_error::_Not_enough_memory; + } + + _Actual_length = __vcrt_GetFinalPathNameByHandleW(_Handle._Get(), _Buf.get() + _Dos_to_nt_prefix_count, + _Buf_count - _Dos_to_nt_prefix_count, FILE_NAME_NORMALIZED | VOLUME_NAME_NT); + if (_Actual_length == 0) { + return __std_win_error{GetLastError()}; + } + + _Actual_length += _Dos_to_nt_prefix_count; + if (_Actual_length <= _Buf_count) { + break; + } + + _Buf_count = _Actual_length; + } + } // close _Handle + + const auto _Ptr = _Buf.get(); + + memcpy(_Ptr, _Dos_to_nt_prefix, _Dos_to_nt_prefix_count * sizeof(wchar_t)); + + // insert null terminator at the last slash + auto _Cursor = _Ptr + _Actual_length; + do { + --_Cursor; // cannot run off start because _Dos_to_nt_prefix contains a backslash + } while (*_Cursor != L'\\'); + + *_Cursor = L'\0'; + if (GetDiskFreeSpaceExW(_Ptr, _Available_c, _Total_bytes_c, _Free_bytes_c)) { + return __std_win_error::_Success; + } + + _Available_c->QuadPart = ~0ull; + _Total_bytes_c->QuadPart = ~0ull; + _Free_bytes_c->QuadPart = ~0ull; + return __std_win_error{GetLastError()}; +} + [[nodiscard]] __std_ulong_and_error __stdcall __std_fs_get_temp_path(wchar_t* const _Target) noexcept { // calls GetTempPathW // If getting the path failed, returns 0 size; otherwise, returns the size of the diff --git a/stl/src/filesystem_space.cpp b/stl/src/filesystem_space.cpp deleted file mode 100644 index 48253129e..000000000 --- a/stl/src/filesystem_space.cpp +++ /dev/null @@ -1,73 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - -// This must be as small as possible, because its contents are -// injected into the msvcprt.lib and msvcprtd.lib import libraries. -// Do not include or define anything else here. -// In particular, basic_string must not be included here. - -// TRANSITION, the code in this file should be moved back to filesystem.cpp -// when a Windows 10 SDK beyond version 1903 is available (see GH-322). - -#include -#include -#include - -#include - -namespace { - static_assert(sizeof(uintmax_t) == sizeof(ULARGE_INTEGER) && alignof(uintmax_t) == alignof(ULARGE_INTEGER), - "Size and alignment must match for reinterpret_cast"); - - [[nodiscard]] __std_win_error _Fs_space_attempt(wchar_t* const _Temp_buffer, const DWORD _Temp_buffer_characters, - const wchar_t* const _Target, uintmax_t* const _Available, uintmax_t* const _Total_bytes, - uintmax_t* const _Free_bytes) noexcept { - if (GetVolumePathNameW(_Target, _Temp_buffer, _Temp_buffer_characters)) { - if (GetDiskFreeSpaceExW(_Temp_buffer, reinterpret_cast(_Available), - reinterpret_cast(_Total_bytes), reinterpret_cast(_Free_bytes))) { - return __std_win_error::_Success; - } - } - - return __std_win_error{GetLastError()}; - } -} // unnamed namespace - -_EXTERN_C -[[nodiscard]] __std_win_error __stdcall __std_fs_space(const wchar_t* const _Target, uintmax_t* const _Available, - uintmax_t* const _Total_bytes, uintmax_t* const _Free_bytes) noexcept { - // get capacity information for the volume on which the file _Target resides - __std_win_error _Last_error; - if (GetFileAttributesW(_Target) == INVALID_FILE_ATTRIBUTES) { - _Last_error = __std_win_error{GetLastError()}; - } else { - { - constexpr DWORD _Static_size = MAX_PATH; - wchar_t _Temp_buf[_Static_size]; - _Last_error = _Fs_space_attempt(_Temp_buf, _Static_size, _Target, _Available, _Total_bytes, _Free_bytes); - if (_Last_error == __std_win_error::_Success) { - return __std_win_error::_Success; - } - } - - if (_Last_error == __std_win_error::_Filename_exceeds_range) { - constexpr DWORD _Dynamic_size = USHRT_MAX + 1; // assuming maximum NT path fits in a UNICODE_STRING - const auto _Temp_buf = _malloc_crt_t(wchar_t, _Dynamic_size); - if (_Temp_buf) { - _Last_error = - _Fs_space_attempt(_Temp_buf.get(), _Dynamic_size, _Target, _Available, _Total_bytes, _Free_bytes); - if (_Last_error == __std_win_error::_Success) { - return __std_win_error::_Success; - } - } else { - _Last_error = __std_win_error::_Not_enough_memory; - } - } - } - - *_Available = ~0ull; - *_Total_bytes = ~0ull; - *_Free_bytes = ~0ull; - return _Last_error; -} -_END_EXTERN_C diff --git a/tests/std/tests/P0218R1_filesystem/test.cpp b/tests/std/tests/P0218R1_filesystem/test.cpp index 13a40e06a..5d5267dc0 100644 --- a/tests/std/tests/P0218R1_filesystem/test.cpp +++ b/tests/std/tests/P0218R1_filesystem/test.cpp @@ -3354,12 +3354,25 @@ void test_space() { EXPECT(info.free == maxValue); EXPECT(info.capacity == maxValue); + const path nonexistent(dir / L"nonexistent"sv); + info = space(nonexistent, ec); + EXPECT(bad(ec)); + EXPECT(info.available == maxValue); + EXPECT(info.free == maxValue); + EXPECT(info.capacity == maxValue); + info = space(LR"(C:\Some\Path\That\Does\Not\Exist)", ec); EXPECT(bad(ec)); EXPECT(info.available == maxValue); EXPECT(info.free == maxValue); EXPECT(info.capacity == maxValue); + info = space(LR"(??malformed??)", ec); + EXPECT(bad(ec)); + EXPECT(info.available == maxValue); + EXPECT(info.free == maxValue); + EXPECT(info.capacity == maxValue); + // Also test VSO-732321 [Feedback] Filesystem::space() doesn't work if the parameter is a relative path info = space(L"."sv, ec); EXPECT(good(ec)); @@ -3367,9 +3380,43 @@ void test_space() { EXPECT(info.free != maxValue); EXPECT(info.capacity != maxValue); - remove(file, ec); - EXPECT(good(ec)); - remove(dir, ec); + const path symDirPresent(dir / L"directory_symlink"sv); + const path symDirAbsent(dir / L"broken_directory_symlink"sv); + const path symFilePresent(dir / L"file_symlink"sv); + const path symFileAbsent(dir / L"broken_file_symlink"sv); + + if ((create_directory_symlink(current_path() / dir, symDirPresent, ec), ec) + || (create_directory_symlink(nonexistent, symDirAbsent, ec), ec) + || (create_symlink(current_path() / file, symFilePresent, ec), ec) + || (create_symlink(nonexistent, symFileAbsent, ec), ec)) { + check_symlink_permissions(ec, L"space symlink edge cases"); + } else { + info = space(symDirPresent, ec); + EXPECT(good(ec)); + EXPECT(info.available != maxValue); + EXPECT(info.free != maxValue); + EXPECT(info.capacity != maxValue); + + info = space(symFilePresent, ec); + EXPECT(good(ec)); + EXPECT(info.available != maxValue); + EXPECT(info.free != maxValue); + EXPECT(info.capacity != maxValue); + + info = space(symDirAbsent, ec); + EXPECT(bad(ec)); + EXPECT(info.available == maxValue); + EXPECT(info.free == maxValue); + EXPECT(info.capacity == maxValue); + + info = space(symFileAbsent, ec); + EXPECT(bad(ec)); + EXPECT(info.available == maxValue); + EXPECT(info.free == maxValue); + EXPECT(info.capacity == maxValue); + } + + remove_all(dir, ec); EXPECT(good(ec)); }