From 71b1639c67b91554420cc38eb4c82323e535c816 Mon Sep 17 00:00:00 2001 From: Jonathan Wakely Date: Mon, 2 Sep 2024 12:16:49 +0100 Subject: [PATCH] libstdc++: Fix error handling in fs::hard_link_count for Windows The recent change to use auto_win_file_handle for std::filesystem::hard_link_count caused a regression. The std::error_code argument should be cleared if no error occurs, but this no longer happens. Add a call to ec.clear() in fs::hard_link_count to fix this. Also change the auto_win_file_handle class to take a reference to the std::error_code and set it if an error occurs, to slightly simplify the control flow in the fs::equiv_files function. libstdc++-v3/ChangeLog: * src/c++17/fs_ops.cc (auto_win_file_handle): Add error_code& member and set it if CreateFileW or GetFileInformationByHandle fails. (fs::equiv_files) [_GLIBCXX_FILESYSTEM_IS_WINDOWS]: Simplify control flow. (fs::hard_link_count) [_GLIBCXX_FILESYSTEM_IS_WINDOWS]: Clear ec on success. * testsuite/27_io/filesystem/operations/hard_link_count.cc: Check error handling. --- libstdc++-v3/src/c++17/fs_ops.cc | 59 ++++++++++++---------- .../27_io/filesystem/operations/hard_link_count.cc | 24 +++++++++ 2 files changed, 57 insertions(+), 26 deletions(-) diff --git a/libstdc++-v3/src/c++17/fs_ops.cc b/libstdc++-v3/src/c++17/fs_ops.cc index 9606afa9f1f..946fefd9e44 100644 --- a/libstdc++-v3/src/c++17/fs_ops.cc +++ b/libstdc++-v3/src/c++17/fs_ops.cc @@ -829,23 +829,37 @@ namespace struct auto_win_file_handle { explicit - auto_win_file_handle(const wchar_t* p) + auto_win_file_handle(const wchar_t* p, std::error_code& ec) noexcept : handle(CreateFileW(p, 0, FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, - 0, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, 0)) - { } + 0, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, 0)), + ec(ec) + { + if (handle == INVALID_HANDLE_VALUE) + ec = std::__last_system_error(); + } ~auto_win_file_handle() { if (*this) CloseHandle(handle); } - explicit operator bool() const + explicit operator bool() const noexcept { return handle != INVALID_HANDLE_VALUE; } - bool get_info() - { return GetFileInformationByHandle(handle, &info); } + bool get_info() noexcept + { + if (GetFileInformationByHandle(handle, &info)) + return true; + ec = std::__last_system_error(); + return false; + } HANDLE handle; BY_HANDLE_FILE_INFORMATION info; + // Like errno, we only set this on error and never clear it. + // This propagates an error_code to the caller when something goes wrong, + // but the caller should not assume a non-zero ec means an error happened + // unless they explicitly cleared it before passing it to our constructor. + std::error_code& ec; }; } #endif @@ -866,23 +880,14 @@ fs::equiv_files([[maybe_unused]] const char_type* p1, const stat_type& st1, if (st1.st_mode != st2.st_mode || st1.st_dev != st2.st_dev) return false; - // Need to use GetFileInformationByHandle to get more info about the files. - auto_win_file_handle h1(p1); - auto_win_file_handle h2(p2); - if (!h1 || !h2) - { - if (!h1 && !h2) - ec = __last_system_error(); - return false; - } - if (!h1.get_info() || !h2.get_info()) - { - ec = __last_system_error(); - return false; - } - return h1.info.dwVolumeSerialNumber == h2.info.dwVolumeSerialNumber - && h1.info.nFileIndexHigh == h2.info.nFileIndexHigh - && h1.info.nFileIndexLow == h2.info.nFileIndexLow; + // Use GetFileInformationByHandle to get more info about the files. + if (auto_win_file_handle h1{p1, ec}) + if (auto_win_file_handle h2{p2, ec}) + if (h1.get_info() && h2.get_info()) + return h1.info.dwVolumeSerialNumber == h2.info.dwVolumeSerialNumber + && h1.info.nFileIndexHigh == h2.info.nFileIndexHigh + && h1.info.nFileIndexLow == h2.info.nFileIndexLow; + return false; #endif // _GLIBCXX_FILESYSTEM_IS_WINDOWS } #endif // NEED_DO_COPY_FILE @@ -1007,10 +1012,12 @@ std::uintmax_t fs::hard_link_count(const path& p, error_code& ec) noexcept { #if _GLIBCXX_FILESYSTEM_IS_WINDOWS - auto_win_file_handle h(p.c_str()); + auto_win_file_handle h(p.c_str(), ec); if (h && h.get_info()) - return static_cast(h.info.nNumberOfLinks); - ec = __last_system_error(); + { + ec.clear(); + return static_cast(h.info.nNumberOfLinks); + } return static_cast(-1); #elif defined _GLIBCXX_HAVE_SYS_STAT_H return do_stat(p, ec, std::mem_fn(&stat_type::st_nlink), diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/hard_link_count.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/hard_link_count.cc index 8b2fb4f190e..4bff39ca308 100644 --- a/libstdc++-v3/testsuite/27_io/filesystem/operations/hard_link_count.cc +++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/hard_link_count.cc @@ -30,8 +30,32 @@ void test01() VERIFY( fs::hard_link_count(p2) == 2 ); } +void +test02() +{ + std::error_code ec; + fs::path p1 = __gnu_test::nonexistent_path(); + VERIFY( fs::hard_link_count(p1, ec) == -1 ); + VERIFY( ec == std::errc::no_such_file_or_directory ); + +#if __cpp_exceptions + try { + fs::hard_link_count(p1); // { dg-warning "ignoring return value" } + VERIFY( false ); + } catch (const fs::filesystem_error& e) { + VERIFY( e.path1() == p1 ); + VERIFY( e.path2().empty() ); + } +#endif + + __gnu_test::scoped_file f1(p1); + fs::hard_link_count(f1.path, ec); // { dg-warning "ignoring return value" } + VERIFY( !ec ); // Should be cleared on success. +} + int main() { test01(); + test02(); } -- 2.11.4.GIT