diff mbox series

[committed] libstdc++: Fix error handling in fs::hard_link_count for Windows

Message ID 20240903151922.2701831-1-jwakely@redhat.com
State New
Headers show
Series [committed] libstdc++: Fix error handling in fs::hard_link_count for Windows | expand

Commit Message

Jonathan Wakely Sept. 3, 2024, 3:19 p.m. UTC 
Tested x86_64-linux. Pushed to trunk.

-- >8 --

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 +++++++++++--------
 .../filesystem/operations/hard_link_count.cc  | 24 ++++++++
 2 files changed, 57 insertions(+), 26 deletions(-)
diff mbox series

Patch

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<uintmax_t>(h.info.nNumberOfLinks);
-  ec = __last_system_error();
+    {
+      ec.clear();
+      return static_cast<uintmax_t>(h.info.nNumberOfLinks);
+    }
   return static_cast<uintmax_t>(-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();
 }