On Fri, May 2, 2025 at 1:12 PM Jonathan Wakely <jwak...@redhat.com> wrote:
> 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.
> ---
>
> Tested x86_64-linux.
>
> libstdc++-v3/src/c++17/fs_path.cc | 10 +++
> .../27_io/filesystem/path/concat/120029.cc | 72 ++++++++++++++++++
> .../filesystem/path/concat/120029.cc | 74 +++++++++++++++++++
> 3 files changed, 156 insertions(+)
> create mode 100644
> libstdc++-v3/testsuite/27_io/filesystem/path/concat/120029.cc
> create mode 100644
> libstdc++-v3/testsuite/experimental/filesystem/path/concat/120029.cc
>
> diff --git a/libstdc++-v3/src/c++17/fs_path.cc
> b/libstdc++-v3/src/c++17/fs_path.cc
> index 6582f10209a..cae16ed607b 100644
> --- 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)
>
Do we want [[unlikely]] also here. Or place these two checks in the
separate function, and then do:
if (_M_overlap(_&p)) [[unlikely]]
return *this += p.native();
> + 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
> index 00000000000..5153d594b50
> --- /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
> index 00000000000..209d96806c6
> --- /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();
> +}
> --
> 2.49.0
>
>
