libstdc++: Fix dangling pointer in fs::path::operator+=(*this) [PR120029]

When concatenating a path we reallocate the left operand's storage to
make room for the new components being added. When the two operands are
the same object, or the right operand is one of the components of the
left operand, the reallocation invalidates the pointers that refer
into the right operand's storage.

The solution in this commit is to detect these aliasing cases and just
do the concatenation in terms of the contained string, as that code
already handles the case where the string aliases the path. The standard
specifies the concatenation in terms of the native() string, so all this
change does is disable the optimized implementation of concatenation for
path objects which attempts to avoid re-parsing the path from the
concatenated string.

The potential loss of performance for this case isn't likely to be an
issue, because concatenating a path with itself (or one of its existing
components) probably isn't a common use case.

The Filesystem TS implementation doesn't have the optimized form of
concatenation and always does it in terms of the native string and
reparsing the whole thing, so doesn't have this bug. A test is added to
confirm that anyway (that test has some slightly different results due
to different behaviour for trailing slashes and implicit "." filenames
in the TS spec).

libstdc++-v3/ChangeLog:

	PR libstdc++/120029
	* src/c++17/fs_path.cc (path::operator+=(const path&)): Handle
	parameters that alias the path or one of its components.
	* testsuite/27_io/filesystem/path/concat/120029.cc: New test.
	* testsuite/experimental/filesystem/path/concat/120029.cc: New
	test.

diff --git a/libstdc++-v3/src/c++17/fs_path.cc b/libstdc++-v3/src/c++17/fs_path.cc
index 6582f10209a084d394bd159cfb06a080025cb353..215afa08ad25f036e0ea7f778f5e9efb3b7d4763 100644 (file)
--- a/libstdc++-v3/src/c++17/fs_path.cc
+++ b/libstdc++-v3/src/c++17/fs_path.cc
@@ -880,6 +880,16 @@ path::operator+=(const path& p)
       return *this;
     }
 
+  // Handle p += p which would otherwise access dangling pointers after
+  // reallocating _M_cmpts and _M_pathname.
+  if (&p == this) [[unlikely]]
+    return *this += p.native();
+  // Handle p += *i where i is in [p.begin(),p.end()), for the same reason.
+  if (_M_type() == _Type::_Multi && p._M_type() != _Type::_Multi)
+    for (const path& cmpt : *this)
+      if (&cmpt == &p) [[unlikely]]
+       return *this += p.native();
+
 #if _GLIBCXX_FILESYSTEM_IS_WINDOWS
   if (_M_type() == _Type::_Root_name
       || (_M_type() == _Type::_Filename && _M_pathname.size() == 1))

diff --git a/libstdc++-v3/testsuite/27_io/filesystem/path/concat/120029.cc b/libstdc++-v3/testsuite/27_io/filesystem/path/concat/120029.cc
new file mode 100644 (file)
index 0000000..5153d59
--- /dev/null
+++ b/libstdc++-v3/testsuite/27_io/filesystem/path/concat/120029.cc
@@ -0,0 +1,72 @@
+// { dg-do run { target c++17 } }
+
+// Bug libstdc++/120029
+// Dangling iterator usage in path::operator+=(const path& p) when this == p
+
+#include <filesystem>
+#include <testsuite_hooks.h>
+
+namespace fs = std::filesystem;
+
+void
+test_root_dir()
+{
+  fs::path p = "/";
+  p += p;
+  p += p;
+  VERIFY( p == "////" );
+  p += p.filename();
+  VERIFY( p == "////" );
+  p += *std::prev(p.end());
+  VERIFY( p == "////" );
+}
+
+void
+test_root_name()
+{
+  fs::path p = "C:/";
+  p += p;
+  p += p;
+  VERIFY( p == "C:/C:/C:/C:/" );
+  p += p.filename();
+  VERIFY( p == "C:/C:/C:/C:/" );
+  p += *std::prev(p.end());
+  VERIFY( p == "C:/C:/C:/C:/" );
+}
+
+void
+test_filename()
+{
+  fs::path p = "file";
+  p += p;
+  p += p;
+  VERIFY( p == "filefilefilefile" );
+  p += p.filename();
+  VERIFY( p == "filefilefilefilefilefilefilefile" );
+  p += *std::prev(p.end());
+  VERIFY( p == "filefilefilefilefilefilefilefilefilefilefilefilefilefilefilefile" );
+}
+
+void
+test_multi()
+{
+  fs::path p = "/home/username/Documents/mu";
+  p += p;
+  p += p;
+  VERIFY( p == "/home/username/Documents/mu/home/username/Documents/mu/home/username/Documents/mu/home/username/Documents/mu" );
+  p += p.filename();
+  VERIFY( p == "/home/username/Documents/mu/home/username/Documents/mu/home/username/Documents/mu/home/username/Documents/mumu" );
+  p += *std::prev(p.end());
+  VERIFY( p == "/home/username/Documents/mu/home/username/Documents/mu/home/username/Documents/mu/home/username/Documents/mumumumu" );
+  auto n = std::distance(p.begin(), p.end());
+  for (int i = 0; i < n; ++i)
+    p += *std::next(p.begin(), i);
+}
+
+int main()
+{
+  test_root_dir();
+  test_root_name();
+  test_filename();
+  test_multi();
+}

diff --git a/libstdc++-v3/testsuite/experimental/filesystem/path/concat/120029.cc b/libstdc++-v3/testsuite/experimental/filesystem/path/concat/120029.cc
new file mode 100644 (file)
index 0000000..209d968
--- /dev/null
+++ b/libstdc++-v3/testsuite/experimental/filesystem/path/concat/120029.cc
@@ -0,0 +1,74 @@
+// { dg-options "-DUSE_FILESYSTEM_TS -lstdc++fs" }
+// { dg-do run { target c++11 } }
+// { dg-require-filesystem-ts "" }
+
+// Bug libstdc++/120029
+// Dangling iterator usage in path::operator+=(const path& p) when this == p
+
+#include <experimental/filesystem>
+#include <testsuite_hooks.h>
+
+namespace fs = std::experimental::filesystem;
+
+void
+test_root_dir()
+{
+  fs::path p = "/";
+  p += p;
+  p += p;
+  VERIFY( p == "////" );
+  p += p.filename();
+  VERIFY( p == "////////" );
+  p += *std::prev(p.end());
+  VERIFY( p == "////////////////" );
+}
+
+void
+test_root_name()
+{
+  fs::path p = "C:/";
+  p += p;
+  p += p;
+  VERIFY( p == "C:/C:/C:/C:/" );
+  p += p.filename(); // For Filesystem TS the filename is "."
+  VERIFY( p == "C:/C:/C:/C:/." );
+  p += *std::prev(p.end());
+  VERIFY( p == "C:/C:/C:/C:/.." );
+}
+
+void
+test_filename()
+{
+  fs::path p = "file";
+  p += p;
+  p += p;
+  VERIFY( p == "filefilefilefile" );
+  p += p.filename();
+  VERIFY( p == "filefilefilefilefilefilefilefile" );
+  p += *std::prev(p.end());
+  VERIFY( p == "filefilefilefilefilefilefilefilefilefilefilefilefilefilefilefile" );
+}
+
+void
+test_multi()
+{
+  fs::path p = "/home/username/Documents/mu";
+  p += p;
+  p += p;
+  VERIFY( p == "/home/username/Documents/mu/home/username/Documents/mu/home/username/Documents/mu/home/username/Documents/mu" );
+  p += p.filename();
+  VERIFY( p == "/home/username/Documents/mu/home/username/Documents/mu/home/username/Documents/mu/home/username/Documents/mumu" );
+  p += *std::prev(p.end());
+  VERIFY( p == "/home/username/Documents/mu/home/username/Documents/mu/home/username/Documents/mu/home/username/Documents/mumumumu" );
+  auto n = std::distance(p.begin(), p.end());
+  for (int i = 0; i < n; ++i)
+    p += *std::next(p.begin(), i);
+}
+
+int main()
+{
+  test_root_dir();
+  test_root_name();
+  test_filename();
+  test_multi();
+}