Fix false positives by filesystem::equivalent on file systems with transient file IDs (#5130)

muellerj2 · StephanTLavavej · web-flow · commit 0053a14dd91f · 2024-12-04T22:38:35.000-08:00
Co-authored-by: Stephan T. Lavavej &lt;stl@nuwen.net&gt;
diff --git 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
@@ -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
@@ -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
@@ -434,7 +434,13 @@ 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
+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
+[[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(
@@ -448,6 +454,37 @@ _Success_(return == __std_win_error::_Success) __std_win_error
     return _Get_file_id_by_handle(_Handle._Get(), reinterpret_cast<FILE_ID_INFO*>(_Id));
 }
 
+[[nodiscard]] __std_fs_equivalent_result __stdcall __std_fs_equivalent(
+    _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(
+        _Left_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 _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};
+    }
+
+    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(
     _In_z_ const wchar_t* const _Symlink_file_name, _In_z_ const wchar_t* const _Target_file_name) noexcept {
     return _Create_symlink(_Symlink_file_name, _Target_file_name, SYMBOLIC_LINK_FLAG_DIRECTORY);
