From 73d968d94561fd521bc6a9af993ed1d1ebba8459 Mon Sep 17 00:00:00 2001 From: Jonathan Wakely Date: Fri, 4 Jan 2019 14:03:59 +0000 Subject: [PATCH] Fix bugs in filesystem::path::lexically_normal() Using path::_List::erase(const_iterator) to remove a non-final component in path::lexically_normal() is a bug, because it leaves the following component with an incorrect _M_pos value. Instead of providing erase members that allow removing components from the middle, replace them with pop_back() and _M_erase_from(const_iterator) which only allow removing elements at the end. Most uses of erase are unaffected, because they only remove elements from the end anyway. The one use of erasure from the middle in lexically_normal() is replaced by calls to pop_back() and/or clearing the last component to leave it as an empty final filename. Also replace the "???" comment in lexically_normal() to document when that branch is taken. * include/bits/fs_path.h (path::_List::erase): Replace both overloads with ... (path::pop_back(), path::_M_erase_from(const_iterator)): New member functions that will only erase elements at the end. * src/filesystem/std-path.cc (path::_List::_Impl::pop_back()): Define. (path::_List::_Impl::_M_erase_from(const_iterator)): Define. (path::_List::operator=(const _List&)): Use _M_erase_from(p) instead of erase(p, end()). (path::_List::pop_back()): Define. (path::_List::_M_erase_from(const_iterator)): Define. (path::operator/=(const path&)): Use pop_back to remove last component and _M_erase_from to remove multiple components. (path::_M_append(basic_string_view)): Likewise. (path::operator+=(const path&)): Likewise. (path::_M_concat(basic_string_view)): Likewise. (path::remove_filename()): Likewise. (path::lexically_normal()): Use _List::_Impl iterators instead of path::iterator. Use pop_back to remove components from the end. Clear trailing filename, instead of using erase(const_iterator) to remove a non-final component. * testsuite/27_io/filesystem/path/generation/normal.cc: Test additional cases. * testsuite/27_io/filesystem/path/generation/normal2.cc: New test. From-SVN: r267576 --- libstdc++-v3/ChangeLog | 24 +++++++ libstdc++-v3/include/bits/fs_path.h | 4 +- libstdc++-v3/src/filesystem/std-path.cc | 72 ++++++++++--------- .../filesystem/path/generation/normal.cc | 2 + .../filesystem/path/generation/normal2.cc | 53 ++++++++++++++ 5 files changed, 121 insertions(+), 34 deletions(-) create mode 100644 libstdc++-v3/testsuite/27_io/filesystem/path/generation/normal2.cc diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog index 483a60224c9..499812cbad6 100644 --- a/libstdc++-v3/ChangeLog +++ b/libstdc++-v3/ChangeLog @@ -1,5 +1,29 @@ 2019-01-04 Jonathan Wakely + * include/bits/fs_path.h (path::_List::erase): Replace both overloads + with ... + (path::pop_back(), path::_M_erase_from(const_iterator)): New member + functions that will only erase elements at the end. + * src/filesystem/std-path.cc (path::_List::_Impl::pop_back()): Define. + (path::_List::_Impl::_M_erase_from(const_iterator)): Define. + (path::_List::operator=(const _List&)): Use _M_erase_from(p) instead + of erase(p, end()). + (path::_List::pop_back()): Define. + (path::_List::_M_erase_from(const_iterator)): Define. + (path::operator/=(const path&)): Use pop_back to remove last component + and _M_erase_from to remove multiple components. + (path::_M_append(basic_string_view)): Likewise. + (path::operator+=(const path&)): Likewise. + (path::_M_concat(basic_string_view)): Likewise. + (path::remove_filename()): Likewise. + (path::lexically_normal()): Use _List::_Impl iterators instead of + path::iterator. Use pop_back to remove components from the end. Clear + trailing filename, instead of using erase(const_iterator) to remove + a non-final component. + * testsuite/27_io/filesystem/path/generation/normal.cc: Test + additional cases. + * testsuite/27_io/filesystem/path/generation/normal2.cc: New test. + * src/filesystem/std-path.cc (path::operator+=(const path&)): Fix incorrect treatment of empty filename after trailing slash. * testsuite/27_io/filesystem/path/concat/path.cc: Test problem case. diff --git a/libstdc++-v3/include/bits/fs_path.h b/libstdc++-v3/include/bits/fs_path.h index 69af50a19f8..34a5d324ce0 100644 --- a/libstdc++-v3/include/bits/fs_path.h +++ b/libstdc++-v3/include/bits/fs_path.h @@ -544,8 +544,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 const value_type& front() const noexcept; const value_type& back() const noexcept; - void erase(const_iterator); - void erase(const_iterator, const_iterator); + void pop_back(); + void _M_erase_from(const_iterator __pos); // erases [__pos,end()) struct _Impl; struct _Impl_deleter diff --git a/libstdc++-v3/src/filesystem/std-path.cc b/libstdc++-v3/src/filesystem/std-path.cc index b7315ad1686..34de52f3a0f 100644 --- a/libstdc++-v3/src/filesystem/std-path.cc +++ b/libstdc++-v3/src/filesystem/std-path.cc @@ -207,22 +207,17 @@ struct path::_List::_Impl void clear() { std::destroy_n(begin(), _M_size); _M_size = 0; } - void erase(const_iterator cpos) + void pop_back() { - iterator pos = begin() + (cpos - begin()); - if (pos + 1 != end()) - std::move(pos + 1, end(), pos); - pos->~_Cmpt(); + back().~_Cmpt(); --_M_size; } - void erase(const_iterator cfirst, const_iterator clast) + void _M_erase_from(const_iterator pos) { - iterator first = begin() + (cfirst - begin()); - iterator last = begin() + (clast - begin()); - if (last != end()) - std::move(last, end(), first); - std::destroy(first + (end() - last), end()); + iterator first = begin() + (pos - begin()); + iterator last = end(); + std::destroy(first, last); _M_size -= last - first; } @@ -288,7 +283,7 @@ path::_List::operator=(const _List& other) impl->_M_size = newsize; } else if (newsize < oldsize) - impl->erase(impl->begin() + newsize, impl->end()); + impl->_M_erase_from(impl->begin() + newsize); std::copy_n(from, minsize, to); type(_Type::_Multi); } @@ -401,15 +396,16 @@ path::_List::back() const noexcept } inline void -path::_List::erase(const_iterator pos) +path::_List::pop_back() { - _M_impl->erase(pos); + __glibcxx_assert(size() > 0); + _M_impl->pop_back(); } inline void -path::_List::erase(const_iterator first, const_iterator last) +path::_List::_M_erase_from(const_iterator pos) { - _M_impl->erase(first, last); + _M_impl->_M_erase_from(pos); } inline void @@ -591,7 +587,10 @@ path::operator/=(const path& __p) { // Remove empty final component if (_M_cmpts._M_impl->back().empty()) - _M_cmpts._M_impl->erase(--output); + { + _M_cmpts.pop_back(); + --output; + } } else if (orig_pathlen != 0) { @@ -629,7 +628,7 @@ path::operator/=(const path& __p) { _M_pathname.resize(orig_pathlen); if (orig_type == _Type::_Multi) - _M_cmpts.erase(_M_cmpts.begin() + orig_size, _M_cmpts.end()); + _M_cmpts._M_erase_from(_M_cmpts.begin() + orig_size); else _M_cmpts.clear(); _M_cmpts.type(orig_type); @@ -774,7 +773,10 @@ path::_M_append(basic_string_view s) { // Remove empty final component if (_M_cmpts._M_impl->back().empty()) - _M_cmpts._M_impl->erase(--output); + { + _M_cmpts.pop_back(); + --output; + } } else if (orig_pathlen != 0) { @@ -818,7 +820,7 @@ path::_M_append(basic_string_view s) { _M_pathname.resize(orig_pathlen); if (orig_type == _Type::_Multi) - _M_cmpts.erase(_M_cmpts.begin() + orig_size, _M_cmpts.end()); + _M_cmpts._M_erase_from(_M_cmpts.begin() + orig_size); else _M_cmpts.clear(); _M_cmpts.type(orig_type); @@ -945,7 +947,8 @@ path::operator+=(const path& p) else if (orig_filenamelen == 0 && it != last) { // Remove empty filename at end of original path. - _M_cmpts.erase(--output); + _M_cmpts.pop_back(); + --output; } if (it != last && it->_M_type() == _Type::_Root_name) @@ -995,7 +998,7 @@ path::operator+=(const path& p) if (orig_type == _Type::_Multi) { if (_M_cmpts.size() > orig_size) - _M_cmpts.erase(_M_cmpts.begin() + orig_size, _M_cmpts.end()); + _M_cmpts._M_erase_from(_M_cmpts.begin() + orig_size); if (orig_filenamelen != -1) { if (_M_cmpts.size() == orig_size) @@ -1182,7 +1185,7 @@ path::_M_concat(basic_string_view s) _M_pathname.resize(orig_pathlen); if (orig_type == _Type::_Multi) { - _M_cmpts.erase(_M_cmpts.begin() + orig_size, _M_cmpts.end()); + _M_cmpts._M_erase_from(_M_cmpts.begin() + orig_size); if (orig_filenamelen != -1) { auto& back = _M_cmpts.back(); @@ -1213,7 +1216,7 @@ path::remove_filename() if (prev->_M_type() == _Type::_Root_dir || prev->_M_type() == _Type::_Root_name) { - _M_cmpts.erase(cmpt); + _M_cmpts.pop_back(); if (_M_cmpts.size() == 1) { _M_cmpts.type(_M_cmpts.front()._M_type()); @@ -1681,25 +1684,30 @@ path::lexically_normal() const // Got a path with a relative path (i.e. at least one non-root // element) and no filename at the end (i.e. empty last element), // so must have a trailing slash. See what is before it. - auto elem = std::prev(ret.end(), 2); + auto elem = ret._M_cmpts.end() - 2; if (elem->has_filename() && !is_dotdot(*elem)) { // Remove the filename before the trailing slash // (equiv. to ret = ret.parent_path().remove_filename()) - if (elem == ret.begin()) + if (elem == ret._M_cmpts.begin()) ret.clear(); else { - ret._M_pathname.erase(elem._M_cur->_M_pos); - // Do we still have a trailing slash? + ret._M_pathname.erase(elem->_M_pos); + // Remove empty filename at the end: + ret._M_cmpts.pop_back(); + // If we still have a trailing non-root dir separator + // then leave an empty filename at the end: if (std::prev(elem)->_M_type() == _Type::_Filename) - ret._M_cmpts.erase(elem._M_cur); - else - ret._M_cmpts.erase(elem._M_cur, ret._M_cmpts.end()); + elem->clear(); + else // remove the component completely: + ret._M_cmpts.pop_back(); } } - else // ??? + else + // Append the ".." to something ending in "../" which happens + // when normalising paths like ".././.." and "../a/../.." ret /= p; } } diff --git a/libstdc++-v3/testsuite/27_io/filesystem/path/generation/normal.cc b/libstdc++-v3/testsuite/27_io/filesystem/path/generation/normal.cc index 320590ccb15..844168352c8 100644 --- a/libstdc++-v3/testsuite/27_io/filesystem/path/generation/normal.cc +++ b/libstdc++-v3/testsuite/27_io/filesystem/path/generation/normal.cc @@ -125,6 +125,8 @@ test03() {"../foo/../foo/.." , ".." }, {"../.f/../f" , "../f" }, {"../f/../.f" , "../.f" }, + {"../.." , "../.." }, + {"../../." , "../.." }, {".././../." , "../.." }, {".././.././" , "../.." }, {"/.." , "/" }, diff --git a/libstdc++-v3/testsuite/27_io/filesystem/path/generation/normal2.cc b/libstdc++-v3/testsuite/27_io/filesystem/path/generation/normal2.cc new file mode 100644 index 00000000000..f78f5dd8a9f --- /dev/null +++ b/libstdc++-v3/testsuite/27_io/filesystem/path/generation/normal2.cc @@ -0,0 +1,53 @@ +// Copyright (C) 2019 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// . + +// { dg-options "-std=gnu++17 -lstdc++fs" } +// { dg-do run { target c++17 } } +// { dg-require-filesystem-ts "" } + +#undef _GLIBCXX_USE_CXX11_ABI +#define _GLIBCXX_USE_CXX11_ABI 0 +#include +#include + +using std::filesystem::path; + +void +compare_paths(path p, std::string expected) +{ +#if defined(_WIN32) && !defined(__CYGWIN__) + for (auto& c : expected) + if (c == '/') + c = '\\'; +#endif + __gnu_test::compare_paths(p, expected); +} + +void +test02() +{ + path p = "./a/b/c/../.././b/c"; + // For the COW string this used to produce incorrect results: + auto norm = p.lexically_normal(); + compare_paths( norm, "a/b/c" ); +} + +int +main() +{ + test02(); +} -- 2.30.2