diff mbox series

[committed] libstdc++: Fix overwriting files with fs::copy_file on Windows

Message ID 20240730142507.1095320-1-jwakely@redhat.com
State New
Headers show
Series [committed] libstdc++: Fix overwriting files with fs::copy_file on Windows | expand

Commit Message

Jonathan Wakely July 30, 2024, 2:19 p.m. UTC
I've pushed this for https://github.com/msys2/MSYS2-packages/issues/1937
but I'm taking a slightly different approach to Björn's original patch.

Instead of adding __detail::equivalent_win32 I'm adding fs::equiv_files
to do the check for both POSIX and Windows. The logic in do_copy_file
should be correct here, and fs::copy also needed an update to fix the
case of overwriting existing directories. I've also added tests.

Tested x86_64-linux and x86_64-w64-mingw32. Pushed to trunk. I'll
backport this after some time on trunk.

-- >8 --

There are no inode numbers on Windows filesystems, so stat_type::st_ino
is always zero and the check for equivalent files in do_copy_file was
incorrectly identifying distinct files as equivalent. This caused
copy_file to incorrectly report errors when trying to overwrite existing
files.

The fs::equivalent function already does the right thing on Windows, so
factor that logic out into a new function that can be reused by
fs::copy_file.

The tests for fs::copy_file were quite inadequate, so this also adds
checks for that function's error conditions.

libstdc++-v3/ChangeLog:

	* src/c++17/fs_ops.cc (auto_win_file_handle): Change constructor
	parameter from const path& to const wchar_t*.
	(fs::equiv_files): New function.
	(fs::equivalent): Use equiv_files.
	* src/filesystem/ops-common.h (fs::equiv_files): Declare.
	(do_copy_file): Use equiv_files.
	* src/filesystem/ops.cc (fs::equiv_files): Define.
	(fs::copy, fs::equivalent): Use equiv_files.
	* testsuite/27_io/filesystem/operations/copy.cc: Test
	overwriting directory contents recursively.
	* testsuite/27_io/filesystem/operations/copy_file.cc: Test
	overwriting existing files.
---
 libstdc++-v3/src/c++17/fs_ops.cc              |  71 ++++++----
 libstdc++-v3/src/filesystem/ops-common.h      |  12 +-
 libstdc++-v3/src/filesystem/ops.cc            |  18 ++-
 .../27_io/filesystem/operations/copy.cc       |   9 ++
 .../27_io/filesystem/operations/copy_file.cc  | 122 ++++++++++++++++++
 5 files changed, 199 insertions(+), 33 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 81227c49dfd..7ffdce67782 100644
--- a/libstdc++-v3/src/c++17/fs_ops.cc
+++ b/libstdc++-v3/src/c++17/fs_ops.cc
@@ -350,7 +350,7 @@  fs::copy(const path& from, const path& to, copy_options options,
   f = make_file_status(from_st);
 
   if (exists(t) && !is_other(t) && !is_other(f)
-      && to_st.st_dev == from_st.st_dev && to_st.st_ino == from_st.st_ino)
+      && fs::equiv_files(from.c_str(), from_st, to.c_str(), to_st, ec))
     {
       ec = std::make_error_code(std::errc::file_exists);
       return;
@@ -829,8 +829,8 @@  namespace
   struct auto_win_file_handle
   {
     explicit
-    auto_win_file_handle(const fs::path& p_)
-    : handle(CreateFileW(p_.c_str(), 0,
+    auto_win_file_handle(const wchar_t* p)
+    : handle(CreateFileW(p, 0,
 			 FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE,
 			 0, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, 0))
     { }
@@ -850,6 +850,44 @@  namespace
 }
 #endif
 
+#ifdef _GLIBCXX_HAVE_SYS_STAT_H
+#ifdef NEED_DO_COPY_FILE // Only define this once, not in cow-ops.o too
+bool
+fs::equiv_files([[maybe_unused]] const char_type* p1, const stat_type& st1,
+		[[maybe_unused]] const char_type* p2, const stat_type& st2,
+		[[maybe_unused]] error_code& ec)
+{
+#if ! _GLIBCXX_FILESYSTEM_IS_WINDOWS
+  // For POSIX the device ID and inode number uniquely identify a file.
+  return st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino;
+#else
+  // For Windows st_ino is not set, so can't be used to distinguish files.
+  // We can compare modes and device IDs as a cheap initial check:
+  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;
+#endif // _GLIBCXX_FILESYSTEM_IS_WINDOWS
+}
+#endif // NEED_DO_COPY_FILE
+#endif // _GLIBCXX_HAVE_SYS_STAT_H
+
 bool
 fs::equivalent(const path& p1, const path& p2, error_code& ec) noexcept
 {
@@ -881,30 +919,7 @@  fs::equivalent(const path& p1, const path& p2, error_code& ec) noexcept
       ec.clear();
       if (is_other(s1) || is_other(s2))
 	return false;
-#if _GLIBCXX_FILESYSTEM_IS_WINDOWS
-      // st_ino is not set, so can't be used to distinguish files
-      if (st1.st_mode != st2.st_mode || st1.st_dev != st2.st_dev)
-	return false;
-
-      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;
-#else
-      return st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino;
-#endif
+      return fs::equiv_files(p1.c_str(), st1, p2.c_str(), st2, ec);
     }
   else if (!exists(s1) || !exists(s2))
     ec = std::make_error_code(std::errc::no_such_file_or_directory);
@@ -992,7 +1007,7 @@  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);
+  auto_win_file_handle h(p.c_str());
   if (h && h.get_info())
     return static_cast<uintmax_t>(h.info.nNumberOfLinks);
   ec = __last_system_error();
diff --git a/libstdc++-v3/src/filesystem/ops-common.h b/libstdc++-v3/src/filesystem/ops-common.h
index d917fddbeb1..1be8d754d6f 100644
--- a/libstdc++-v3/src/filesystem/ops-common.h
+++ b/libstdc++-v3/src/filesystem/ops-common.h
@@ -303,7 +303,6 @@  namespace __gnu_posix
   {
     bool skip, update, overwrite;
   };
-
 #endif // _GLIBCXX_HAVE_SYS_STAT_H
 
 } // namespace filesystem
@@ -327,6 +326,12 @@  _GLIBCXX_BEGIN_NAMESPACE_FILESYSTEM
 	   std::error_code&);
 
 
+  // Test whether two files are the same file.
+  bool
+  equiv_files(const char_type*, const stat_type&,
+	      const char_type*, const stat_type&,
+	      error_code&);
+
   inline file_type
   make_file_type(const stat_type& st) noexcept
   {
@@ -400,6 +405,7 @@  _GLIBCXX_BEGIN_NAMESPACE_FILESYSTEM
     return true;
   }
 #endif
+
 #if defined _GLIBCXX_USE_SENDFILE && ! defined _GLIBCXX_FILESYSTEM_IS_WINDOWS
   bool
   copy_file_sendfile(int fd_in, int fd_out, size_t length) noexcept
@@ -428,6 +434,7 @@  _GLIBCXX_BEGIN_NAMESPACE_FILESYSTEM
     return true;
   }
 #endif
+
   bool
   do_copy_file(const char_type* from, const char_type* to,
 	       std::filesystem::copy_options_existing_file options,
@@ -489,8 +496,7 @@  _GLIBCXX_BEGIN_NAMESPACE_FILESYSTEM
 	    return false;
 	  }
 
-	if (to_st->st_dev == from_st->st_dev
-	    && to_st->st_ino == from_st->st_ino)
+	if (equiv_files(from, *from_st, to, *to_st, ec))
 	  {
 	    ec = std::make_error_code(std::errc::file_exists);
 	    return false;
diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc
index 4d23a804da0..c430f58c47a 100644
--- a/libstdc++-v3/src/filesystem/ops.cc
+++ b/libstdc++-v3/src/filesystem/ops.cc
@@ -257,6 +257,20 @@  namespace
 
 } // namespace
 
+#ifdef _GLIBCXX_HAVE_SYS_STAT_H
+#ifdef NEED_DO_COPY_FILE // Only define this once, not in cow-ops.o too
+bool
+fs::equiv_files([[maybe_unused]] const char_type* p1, const stat_type& st1,
+		[[maybe_unused]] const char_type* p2, const stat_type& st2,
+		[[maybe_unused]] error_code& ec)
+{
+  // For POSIX the device ID and inode number uniquely identify a file.
+  // This doesn't work on Windows (see equiv_files in src/c++17/fs_ops.cc).
+  return st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino;
+}
+#endif
+#endif
+
 void
 fs::copy(const path& from, const path& to, copy_options options,
 	 error_code& ec) noexcept
@@ -293,7 +307,7 @@  fs::copy(const path& from, const path& to, copy_options options,
   f = make_file_status(from_st);
 
   if (exists(t) && !is_other(t) && !is_other(f)
-      && to_st.st_dev == from_st.st_dev && to_st.st_ino == from_st.st_ino)
+      && fs::equiv_files(from.c_str(), from_st, to.c_str(), to_st, ec))
     {
       ec = std::make_error_code(std::errc::file_exists);
       return;
@@ -763,7 +777,7 @@  fs::equivalent(const path& p1, const path& p2, error_code& ec) noexcept
       ec.clear();
       if (is_other(s1) || is_other(s2))
 	return false;
-      return st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino;
+      return fs::equiv_files(p1.c_str(), st1, p2.c_str(), st2, ec);
     }
   else if (!exists(s1) || !exists(s2))
     ec = std::make_error_code(std::errc::no_such_file_or_directory);
diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc
index 777a9b8402e..893f32297ae 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc
@@ -216,6 +216,15 @@  test_pr99290()
   }
 #endif
 
+  // https://github.com/msys2/MSYS2-packages/issues/1937
+  auto copy_opts
+    = fs::copy_options::overwrite_existing | fs::copy_options::recursive;
+  copy(source, dest, copy_opts, ec);
+  VERIFY( !ec );
+
+  auto ch = std::ifstream{dest/"file"}.get();
+  VERIFY( ch == 'a' );
+
   remove_all(dir);
 }
 
diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/copy_file.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/copy_file.cc
index e3a1219fa6e..af643151044 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/operations/copy_file.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/copy_file.cc
@@ -76,8 +76,130 @@  test01()
   remove(to);
 }
 
+void
+test_directory()
+{
+  std::error_code ec;
+
+  auto dir = __gnu_test::nonexistent_path();
+  auto file = __gnu_test::nonexistent_path();
+
+  // error condition: is_regular_file(from) is false
+  create_directory(dir);
+  bool b = copy_file(dir, file, ec);
+  VERIFY( !b );
+  VERIFY( ec == std::errc::invalid_argument );
+  VERIFY( !exists(file) );
+  VERIFY( is_directory(dir) );
+
+  ec = std::make_error_code(std::errc::address_in_use);
+  // error condition: exists(to) is true and is_regular_file(to) is false
+  std::ofstream{file} << "regular file";
+  b = copy_file(file, dir, ec);
+  VERIFY( !b );
+  VERIFY( ec == std::errc::invalid_argument );
+  VERIFY( exists(file) );
+  VERIFY( is_directory(dir) );
+
+  remove(dir);
+  remove(file);
+}
+
+void
+test_existing()
+{
+  using std::filesystem::copy_options;
+  std::error_code ec;
+
+  auto from = __gnu_test::nonexistent_path();
+  auto to = from;
+
+  // error condition: exists(to) is true and equivalent(from, to) is true
+  std::ofstream{from} << "source";
+  bool b = copy_file(from, to, ec);
+  VERIFY( !b );
+  VERIFY( ec == std::errc::file_exists );
+
+  to = __gnu_test::nonexistent_path();
+  std::ofstream{to} << "overwrite me";
+
+  // error condition: exists(to) is true and options == copy_options::none
+  b = copy_file(from, to, ec);
+  VERIFY( !b );
+  VERIFY( ec == std::errc::file_exists );
+  VERIFY( file_size(to) != file_size(from) );
+
+#if __cpp_exceptions
+  try
+  {
+    (void) copy_file(from, to);
+    VERIFY(false);
+  }
+  catch (const std::filesystem::filesystem_error& e)
+  {
+    std::string_view msg = e.what();
+    VERIFY( msg.find(from.string()) != msg.npos );
+    VERIFY( msg.find(to.string()) != msg.npos );
+    VERIFY( e.code() == std::errc::file_exists );
+  }
+  VERIFY( file_size(to) == 12 );
+#endif
+
+  b = copy_file(from, to, copy_options::skip_existing, ec);
+  VERIFY( !b );
+  VERIFY( !ec );
+  VERIFY( file_size(to) != file_size(from) );
+
+  b = copy_file(from, to, copy_options::skip_existing); // doesn't throw
+  VERIFY( !b );
+  VERIFY( file_size(to) != file_size(from) );
+
+  b = copy_file(from, to, copy_options::overwrite_existing, ec);
+  VERIFY( b );
+  VERIFY( !ec );
+  VERIFY( file_size(to) == file_size(from) );
+
+  std::ofstream{to} << "overwrite me again";
+  b = copy_file(from, to, copy_options::overwrite_existing); // doesn't throw
+  VERIFY( b );
+  VERIFY( file_size(to) == file_size(from) );
+
+  ec = std::make_error_code(std::errc::address_in_use);
+  auto time = last_write_time(from);
+  std::ofstream{to} << "touched";
+  last_write_time(to, time + std::chrono::seconds(60));
+  b = copy_file(from, to, copy_options::update_existing, ec);
+  VERIFY( !b );
+  VERIFY( !ec );
+  VERIFY( file_size(to) != file_size(from) );
+
+  b = copy_file(from, to, copy_options::update_existing); // doesn't throw
+  VERIFY( !b );
+  VERIFY( file_size(to) != file_size(from) );
+
+  ec = std::make_error_code(std::errc::address_in_use);
+  time = last_write_time(to);
+  std::ofstream{from} << "touched more recently";
+  last_write_time(from, time + std::chrono::seconds(60));
+  b = copy_file(from, to, copy_options::update_existing, ec);
+  VERIFY( b );
+  VERIFY( !ec );
+  VERIFY( file_size(to) == file_size(from) );
+
+  time = last_write_time(to);
+  std::ofstream{from} << "touched even more recently";
+  last_write_time(from, time + std::chrono::seconds(60));
+  b = copy_file(from, to, copy_options::update_existing); // doesn't throw
+  VERIFY( b );
+  VERIFY( file_size(to) == file_size(from) );
+
+  remove(from);
+  remove(to);
+}
+
 int
 main()
 {
   test01();
+  test_existing();
 }