From 50d54266a4627eebb9638888c6574406ac8b22eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20M=C3=BCller?= Date: Sun, 24 Nov 2024 18:36:02 +0100 Subject: [PATCH 1/5] Fix false positives by `filesystem::equivalent` on file systems with transient file IDs --- stl/inc/filesystem | 31 +++++++------------------------ stl/inc/xfilesystem_abi.h | 14 +++++++------- stl/src/filesys.cpp | 39 +++++++++++++++++++++++---------------- stl/src/filesystem.cpp | 33 +++++++++++++++++++++++++-------- 4 files changed, 62 insertions(+), 55 deletions(-) diff --git a/stl/inc/filesystem b/stl/inc/filesystem index 5c321608490..d9a4a547b12 100644 --- a/stl/inc/filesystem +++ b/stl/inc/filesystem @@ -3431,38 +3431,21 @@ namespace filesystem { return _STD filesystem::copy_file(_From, _To, copy_options::none); } - _NODISCARD inline pair<__std_win_error, bool> _Equivalent( - const wchar_t* const _Lhs, const wchar_t* const _Rhs) noexcept { - __std_fs_file_id _Left_id; - __std_fs_file_id _Right_id; - auto _Last_error = __std_fs_get_file_id(&_Left_id, _Lhs); - if (_Last_error != __std_win_error::_Success) { - return {_Last_error, false}; - } - - _Last_error = __std_fs_get_file_id(&_Right_id, _Rhs); - if (_Last_error != __std_win_error::_Success) { - return {_Last_error, false}; - } - - return {__std_win_error::_Success, _CSTD memcmp(&_Left_id, &_Right_id, sizeof(__std_fs_file_id)) == 0}; - } - _EXPORT_STD _NODISCARD inline bool equivalent(const path& _Lhs, const path& _Rhs) { // test if the paths _Lhs and _Rhs refer to the same file - const auto _Result = _Equivalent(_Lhs.c_str(), _Rhs.c_str()); - if (_Result.first != __std_win_error::_Success) { - _Throw_fs_error("equivalent", _Result.first, _Lhs, _Rhs); + const auto _Result = __std_fs_equivalent(_Lhs.c_str(), _Rhs.c_str()); + if (_Result._Error != __std_win_error::_Success) { + _Throw_fs_error("equivalent", _Result._Error, _Lhs, _Rhs); } - return _Result.second; + return _Result._Equivalent; } _EXPORT_STD _NODISCARD inline bool equivalent(const path& _Lhs, const path& _Rhs, error_code& _Ec) noexcept { // test if the paths _Lhs and _Rhs refer to the same file - const auto _Result = _Equivalent(_Lhs.c_str(), _Rhs.c_str()); - _Ec = _Make_ec(_Result.first); - return _Result.second; + const auto _Result = __std_fs_equivalent(_Lhs.c_str(), _Rhs.c_str()); + _Ec = _Make_ec(_Result._Error); + return _Result._Equivalent; } _EXPORT_STD _NODISCARD inline file_status status(const path& _Path); diff --git a/stl/inc/xfilesystem_abi.h b/stl/inc/xfilesystem_abi.h index 2813e99951a..f00eaceedb8 100644 --- a/stl/inc/xfilesystem_abi.h +++ b/stl/inc/xfilesystem_abi.h @@ -232,11 +232,6 @@ struct __std_fs_convert_result { __std_win_error _Err; }; -struct __std_fs_file_id { // typedef struct _FILE_ID_INFO { - unsigned long long _Volume_serial_number; // ULONGLONG VolumeSerialNumber; - unsigned char _Id[16]; // FILE_ID_128 FileId; -}; // } FILE_ID_INFO, ...; - enum class __std_fs_copy_options { _None = 0x0, @@ -303,8 +298,13 @@ _NODISCARD __std_fs_convert_result __stdcall __std_fs_convert_wide_to_narrow_rep _In_ __std_code_page _Code_page, _In_reads_(_Input_len) const wchar_t* _Input_str, _In_ int _Input_len, _Out_writes_opt_(_Output_len) char* _Output_str, _In_ int _Output_len) noexcept; -_NODISCARD _Success_(return == __std_win_error::_Success) __std_win_error - __stdcall __std_fs_get_file_id(_Out_ __std_fs_file_id* _Id, _In_z_ const wchar_t* _Path) noexcept; +struct __std_fs_equivalent_result { + bool _Equivalent; + __std_win_error _Error; +}; + +_NODISCARD __std_fs_equivalent_result __stdcall __std_fs_equivalent( + _In_z_ const wchar_t* _Path1, _In_z_ const wchar_t* _Path2) noexcept; _NODISCARD __std_win_error __stdcall __std_fs_set_last_write_time( _In_ long long _Last_write_filetime, _In_z_ const wchar_t* _Path) noexcept; diff --git a/stl/src/filesys.cpp b/stl/src/filesys.cpp index 0bebdc16033..daa5791f577 100644 --- a/stl/src/filesys.cpp +++ b/stl/src/filesys.cpp @@ -320,22 +320,26 @@ _FS_DLL space_info __CLRCALL_PURE_OR_CDECL _Statvfs(const wchar_t* _Fname) noexc _FS_DLL int __CLRCALL_PURE_OR_CDECL _Equivalent( const wchar_t* _Fname1, const wchar_t* _Fname2) noexcept { // test for equivalent file names + // See GH-3571: File IDs are only guaranteed to be unique and stable while handles remain open #ifdef _CRT_APP _FILE_ID_INFO _Info1 = {0}; _FILE_ID_INFO _Info2 = {0}; bool _Ok1 = false; bool _Ok2 = false; - HANDLE _Handle = _FilesysOpenFile(_Fname1, FILE_READ_ATTRIBUTES, FILE_FLAG_BACKUP_SEMANTICS); - if (_Handle != INVALID_HANDLE_VALUE) { // get file1 info - _Ok1 = GetFileInformationByHandleEx(_Handle, FileIdInfo, &_Info1, sizeof(_Info1)) != 0; - CloseHandle(_Handle); + HANDLE _Handle1 = _FilesysOpenFile(_Fname1, FILE_READ_ATTRIBUTES, FILE_FLAG_BACKUP_SEMANTICS); + if (_Handle1 != INVALID_HANDLE_VALUE) { // get file1 info + _Ok1 = GetFileInformationByHandleEx(_Handle1, FileIdInfo, &_Info1, sizeof(_Info1)) != 0; } - _Handle = _FilesysOpenFile(_Fname2, FILE_READ_ATTRIBUTES, FILE_FLAG_BACKUP_SEMANTICS); - if (_Handle != INVALID_HANDLE_VALUE) { // get file2 info - _Ok2 = GetFileInformationByHandleEx(_Handle, FileIdInfo, &_Info2, sizeof(_Info2)) != 0; - CloseHandle(_Handle); + HANDLE _Handle2 = _FilesysOpenFile(_Fname2, FILE_READ_ATTRIBUTES, FILE_FLAG_BACKUP_SEMANTICS); + if (_Handle2 != INVALID_HANDLE_VALUE) { // get file2 info + _Ok2 = GetFileInformationByHandleEx(_Handle2, FileIdInfo, &_Info2, sizeof(_Info2)) != 0; + CloseHandle(_Handle2); + } + + if (_Handle1 != INVALID_HANDLE_VALUE) { + CloseHandle(_Handle1); } if (!_Ok1 && !_Ok2) { @@ -351,16 +355,19 @@ _FS_DLL int __CLRCALL_PURE_OR_CDECL _Equivalent( bool _Ok1 = false; bool _Ok2 = false; - HANDLE _Handle = _FilesysOpenFile(_Fname1, FILE_READ_ATTRIBUTES, FILE_FLAG_BACKUP_SEMANTICS); - if (_Handle != INVALID_HANDLE_VALUE) { // get file1 info - _Ok1 = GetFileInformationByHandle(_Handle, &_Info1) != 0; - CloseHandle(_Handle); + HANDLE _Handle1 = _FilesysOpenFile(_Fname1, FILE_READ_ATTRIBUTES, FILE_FLAG_BACKUP_SEMANTICS); + if (_Handle1 != INVALID_HANDLE_VALUE) { // get file1 info + _Ok1 = GetFileInformationByHandle(_Handle1, &_Info1) != 0; + } + + HANDLE _Handle2 = _FilesysOpenFile(_Fname2, FILE_READ_ATTRIBUTES, FILE_FLAG_BACKUP_SEMANTICS); + if (_Handle2 != INVALID_HANDLE_VALUE) { // get file2 info + _Ok2 = GetFileInformationByHandle(_Handle2, &_Info2) != 0; + CloseHandle(_Handle2); } - _Handle = _FilesysOpenFile(_Fname2, FILE_READ_ATTRIBUTES, FILE_FLAG_BACKUP_SEMANTICS); - if (_Handle != INVALID_HANDLE_VALUE) { // get file2 info - _Ok2 = GetFileInformationByHandle(_Handle, &_Info2) != 0; - CloseHandle(_Handle); + if (_Handle1 != INVALID_HANDLE_VALUE) { + CloseHandle(_Handle1); } if (!_Ok1 && !_Ok2) { diff --git a/stl/src/filesystem.cpp b/stl/src/filesystem.cpp index ba05446a1d7..1263de8f2a1 100644 --- a/stl/src/filesystem.cpp +++ b/stl/src/filesystem.cpp @@ -434,18 +434,35 @@ void __stdcall __std_fs_directory_iterator_close(_In_ const __std_fs_dir_handle return __vcp_Copyfile(_Source, _Target, /* _Fail_if_exists = */ false); } -_Success_(return == __std_win_error::_Success) __std_win_error - __stdcall __std_fs_get_file_id(_Out_ __std_fs_file_id* const _Id, _In_z_ const wchar_t* const _Path) noexcept { +_NODISCARD __std_fs_equivalent_result __stdcall __std_fs_equivalent( + _In_z_ const wchar_t* _Left_path, _In_z_ const wchar_t* _Right_path) noexcept { + // See GH-3571: File IDs are only guaranteed to be unique and stable while handles remain open __std_win_error _Last_error; - const _STD _Fs_file _Handle( - _Path, __std_access_rights::_File_read_attributes, __std_fs_file_flags::_Backup_semantics, &_Last_error); + const _STD _Fs_file _Left_handle( + _Left_path, __std_access_rights::_File_read_attributes, __std_fs_file_flags::_Backup_semantics, &_Last_error); if (_Last_error != __std_win_error::_Success) { - return _Last_error; + return {false, _Last_error}; + } + + FILE_ID_INFO _Left_info; + _Last_error = _Get_file_id_by_handle(_Left_handle._Get(), &_Left_info); + if (_Last_error != __std_win_error::_Success) { + return {false, _Last_error}; + } + + const _STD _Fs_file _Right_handle( + _Right_path, __std_access_rights::_File_read_attributes, __std_fs_file_flags::_Backup_semantics, &_Last_error); + if (_Last_error != __std_win_error::_Success) { + return {false, _Last_error}; + } + + FILE_ID_INFO _Right_info; + _Last_error = _Get_file_id_by_handle(_Right_handle._Get(), &_Right_info); + if (_Last_error != __std_win_error::_Success) { + return {false, _Last_error}; } - static_assert(sizeof(FILE_ID_INFO) == sizeof(__std_fs_file_id)); - static_assert(alignof(FILE_ID_INFO) == alignof(__std_fs_file_id)); - return _Get_file_id_by_handle(_Handle._Get(), reinterpret_cast(_Id)); + return {_CSTD memcmp(&_Left_info, &_Right_info, sizeof(FILE_ID_INFO)) == 0, _Last_error}; } [[nodiscard]] __std_win_error __stdcall __std_fs_create_directory_symbolic_link( From a87b43f7bc2c67ea7d01378d909109a4e954cf00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20M=C3=BCller?= Date: Sun, 24 Nov 2024 21:11:44 +0100 Subject: [PATCH 2/5] restore __std_fs_get_file_id() --- stl/src/filesystem.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/stl/src/filesystem.cpp b/stl/src/filesystem.cpp index 1263de8f2a1..4254e8a52dd 100644 --- a/stl/src/filesystem.cpp +++ b/stl/src/filesystem.cpp @@ -434,6 +434,26 @@ void __stdcall __std_fs_directory_iterator_close(_In_ const __std_fs_dir_handle return __vcp_Copyfile(_Source, _Target, /* _Fail_if_exists = */ false); } +struct __std_fs_file_id { // typedef struct _FILE_ID_INFO { + unsigned long long _Volume_serial_number; // ULONGLONG VolumeSerialNumber; + unsigned char _Id[16]; // FILE_ID_128 FileId; +}; // } FILE_ID_INFO, ...; + +// TRANSITION, ABI: preserved for binary compatibility +_Success_(return == __std_win_error::_Success) __std_win_error + __stdcall __std_fs_get_file_id(_Out_ __std_fs_file_id* const _Id, _In_z_ const wchar_t* const _Path) noexcept { + __std_win_error _Last_error; + const _STD _Fs_file _Handle( + _Path, __std_access_rights::_File_read_attributes, __std_fs_file_flags::_Backup_semantics, &_Last_error); + if (_Last_error != __std_win_error::_Success) { + return _Last_error; + } + + static_assert(sizeof(FILE_ID_INFO) == sizeof(__std_fs_file_id)); + static_assert(alignof(FILE_ID_INFO) == alignof(__std_fs_file_id)); + return _Get_file_id_by_handle(_Handle._Get(), reinterpret_cast(_Id)); +} + _NODISCARD __std_fs_equivalent_result __stdcall __std_fs_equivalent( _In_z_ const wchar_t* _Left_path, _In_z_ const wchar_t* _Right_path) noexcept { // See GH-3571: File IDs are only guaranteed to be unique and stable while handles remain open From 34368ed6c0ea4b0d89cc4df40e1497b1edf74eac Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 2 Dec 2024 09:54:58 -0800 Subject: [PATCH 3/5] Mark definitions as `[[nodiscard]]`. --- stl/src/filesystem.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stl/src/filesystem.cpp b/stl/src/filesystem.cpp index 4254e8a52dd..ae2c9de9d16 100644 --- a/stl/src/filesystem.cpp +++ b/stl/src/filesystem.cpp @@ -440,7 +440,7 @@ struct __std_fs_file_id { // typedef struct _FILE_ID_INFO { }; // } FILE_ID_INFO, ...; // TRANSITION, ABI: preserved for binary compatibility -_Success_(return == __std_win_error::_Success) __std_win_error +[[nodiscard]] _Success_(return == __std_win_error::_Success) __std_win_error __stdcall __std_fs_get_file_id(_Out_ __std_fs_file_id* const _Id, _In_z_ const wchar_t* const _Path) noexcept { __std_win_error _Last_error; const _STD _Fs_file _Handle( @@ -454,7 +454,7 @@ _Success_(return == __std_win_error::_Success) __std_win_error return _Get_file_id_by_handle(_Handle._Get(), reinterpret_cast(_Id)); } -_NODISCARD __std_fs_equivalent_result __stdcall __std_fs_equivalent( +[[nodiscard]] __std_fs_equivalent_result __stdcall __std_fs_equivalent( _In_z_ const wchar_t* _Left_path, _In_z_ const wchar_t* _Right_path) noexcept { // See GH-3571: File IDs are only guaranteed to be unique and stable while handles remain open __std_win_error _Last_error; From edf968c993cbfd0df56c960f76ab10f5f0f2ba0c Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 2 Dec 2024 09:58:42 -0800 Subject: [PATCH 4/5] Add top-level const. --- stl/src/filesystem.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/src/filesystem.cpp b/stl/src/filesystem.cpp index ae2c9de9d16..e200481b946 100644 --- a/stl/src/filesystem.cpp +++ b/stl/src/filesystem.cpp @@ -455,7 +455,7 @@ struct __std_fs_file_id { // typedef struct _FILE_ID_INFO { } [[nodiscard]] __std_fs_equivalent_result __stdcall __std_fs_equivalent( - _In_z_ const wchar_t* _Left_path, _In_z_ const wchar_t* _Right_path) noexcept { + _In_z_ const wchar_t* const _Left_path, _In_z_ const wchar_t* const _Right_path) noexcept { // See GH-3571: File IDs are only guaranteed to be unique and stable while handles remain open __std_win_error _Last_error; const _STD _Fs_file _Left_handle( From 487820594451dd86fe3196b01b63e020e079a826 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 2 Dec 2024 10:00:06 -0800 Subject: [PATCH 5/5] `_Last_error` is `__std_win_error::_Success` at the end. --- stl/src/filesystem.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/src/filesystem.cpp b/stl/src/filesystem.cpp index e200481b946..25512bdfcd4 100644 --- a/stl/src/filesystem.cpp +++ b/stl/src/filesystem.cpp @@ -482,7 +482,7 @@ struct __std_fs_file_id { // typedef struct _FILE_ID_INFO { return {false, _Last_error}; } - return {_CSTD memcmp(&_Left_info, &_Right_info, sizeof(FILE_ID_INFO)) == 0, _Last_error}; + return {_CSTD memcmp(&_Left_info, &_Right_info, sizeof(FILE_ID_INFO)) == 0, __std_win_error::_Success}; } [[nodiscard]] __std_win_error __stdcall __std_fs_create_directory_symbolic_link(