This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[patch] Fix exception-safety issue in std::forward_list
- From: Jonathan Wakely <jwakely at redhat dot com>
- To: libstdc++ at gcc dot gnu dot org, gcc-patches at gcc dot gnu dot org
- Date: Wed, 17 Jun 2015 18:45:53 +0100
- Subject: [patch] Fix exception-safety issue in std::forward_list
- Authentication-results: sourceware.org; auth=none
There's a potential memory leak in the allocator-extended move
constructor of forward_list, if the allocator parameter is not equal
to the rvalue list's allocator then new list nodes must be allocated,
but if doing so throws then the already-allocated nodes are not freed.
Rather than adding a try-catch block this moves the allocations out of
the _Fwd_list_base constructor and into the derived constructor, so
that if an exception is thrown the base class will clean everything
up.
This also splits out the default constructor from the one taking an
allocator, so the default constructor is not explicit (as per the
resolution of LWG 2193) and can be conditionally-noexcept (as per LWG
2455) but the one taking an allocator is explicit and noexcept
(because copying allocators can't throw).
There are also some small optimisations:
The base constructors that take an allocator can take it by rvalue
reference and move from it, because we always have to construct a
temporary _Node_alloc_type from the user-supplied allocator.
The call to std::swap in _M_move_assign can be replaced with two
assignments, because we know one of the pointers is null so there's no
need to use a temporary variable to preserve its value.
Tested powerpc64le-linux, committed to trunk.
commit 2de95f71039003d692934065799fd3fc66155d2f
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Wed Jun 17 16:48:59 2015 +0100
* include/bits/forward_list.h
(_Fwd_list_base(const _Node_alloc_type&)): Change parameter to
rvalue-reference.
(_Fwd_list_base(_Fwd_list_base&&, const _Node_alloc_type&)): Likewise.
(forward_list(const _Alloc&)): Split default constructor out to
separate function.
(forward_list(forward_list&&, const _Alloc&)): Move elements if base
class didn't do so.
(forward_list::_M_move_assign(forward_list&&, true_type)): Replace
swap call with two assignments.
* include/bits/forward_list.tcc
(_Fwd_list_base(_Fwd_list_base&&, const _Node_alloc_type&)): Don't
move elements when allocators are not equal.
* include/debug/forward_list (forward_list(const allocator_type&)):
Split default constructor out to separate function.
* include/profile/forward_list (forward_list(const _Alloc&)):
Likewise.
diff --git a/libstdc++-v3/include/bits/forward_list.h b/libstdc++-v3/include/bits/forward_list.h
index d611ade..57c2d7b 100644
--- a/libstdc++-v3/include/bits/forward_list.h
+++ b/libstdc++-v3/include/bits/forward_list.h
@@ -314,10 +314,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
_Fwd_list_base()
: _M_impl() { }
- _Fwd_list_base(const _Node_alloc_type& __a)
- : _M_impl(__a) { }
+ _Fwd_list_base(_Node_alloc_type&& __a)
+ : _M_impl(std::move(__a)) { }
- _Fwd_list_base(_Fwd_list_base&& __lst, const _Node_alloc_type& __a);
+ _Fwd_list_base(_Fwd_list_base&& __lst, _Node_alloc_type&& __a);
_Fwd_list_base(_Fwd_list_base&& __lst)
: _M_impl(std::move(__lst._M_get_Node_allocator()))
@@ -435,13 +435,22 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
/**
* @brief Creates a %forward_list with no elements.
+ */
+ forward_list()
+ noexcept(is_nothrow_default_constructible<_Node_alloc_type>::value)
+ : _Base()
+ { }
+
+ /**
+ * @brief Creates a %forward_list with no elements.
* @param __al An allocator object.
*/
explicit
- forward_list(const _Alloc& __al = _Alloc())
+ forward_list(const _Alloc& __al) noexcept
: _Base(_Node_alloc_type(__al))
{ }
+
/**
* @brief Copy constructor with allocator argument.
* @param __list Input list to copy.
@@ -459,7 +468,13 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
forward_list(forward_list&& __list, const _Alloc& __al)
noexcept(_Node_alloc_traits::_S_always_equal())
: _Base(std::move(__list), _Node_alloc_type(__al))
- { }
+ {
+ // If __list is not empty it means its allocator is not equal to __a,
+ // so we need to move from each element individually.
+ insert_after(cbefore_begin(),
+ std::__make_move_if_noexcept_iterator(__list.begin()),
+ std::__make_move_if_noexcept_iterator(__list.end()));
+ }
/**
* @brief Creates a %forward_list with default constructed elements.
@@ -1248,8 +1263,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
_M_move_assign(forward_list&& __list, std::true_type) noexcept
{
clear();
- std::swap(this->_M_impl._M_head._M_next,
- __list._M_impl._M_head._M_next);
+ this->_M_impl._M_head._M_next = __list._M_impl._M_head._M_next;
+ __list._M_impl._M_head._M_next = nullptr;
std::__alloc_on_move(this->_M_get_Node_allocator(),
__list._M_get_Node_allocator());
}
diff --git a/libstdc++-v3/include/bits/forward_list.tcc b/libstdc++-v3/include/bits/forward_list.tcc
index 1e1a02e..472020c 100644
--- a/libstdc++-v3/include/bits/forward_list.tcc
+++ b/libstdc++-v3/include/bits/forward_list.tcc
@@ -36,28 +36,16 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
template<typename _Tp, typename _Alloc>
_Fwd_list_base<_Tp, _Alloc>::
- _Fwd_list_base(_Fwd_list_base&& __lst, const _Node_alloc_type& __a)
- : _M_impl(__a)
+ _Fwd_list_base(_Fwd_list_base&& __lst, _Node_alloc_type&& __a)
+ : _M_impl(std::move(__a))
{
- if (__lst._M_get_Node_allocator() == __a)
+ if (__lst._M_get_Node_allocator() == _M_get_Node_allocator())
{
this->_M_impl._M_head._M_next = __lst._M_impl._M_head._M_next;
__lst._M_impl._M_head._M_next = 0;
}
else
- {
- this->_M_impl._M_head._M_next = 0;
- _Fwd_list_node_base* __to = &this->_M_impl._M_head;
- _Node* __curr = static_cast<_Node*>(__lst._M_impl._M_head._M_next);
-
- while (__curr)
- {
- __to->_M_next =
- _M_create_node(std::move_if_noexcept(*__curr->_M_valptr()));
- __to = __to->_M_next;
- __curr = static_cast<_Node*>(__curr->_M_next);
- }
- }
+ this->_M_impl._M_head._M_next = 0;
}
template<typename _Tp, typename _Alloc>
diff --git a/libstdc++-v3/include/debug/forward_list b/libstdc++-v3/include/debug/forward_list
index d22c22f..2b42a3f 100644
--- a/libstdc++-v3/include/debug/forward_list
+++ b/libstdc++-v3/include/debug/forward_list
@@ -204,8 +204,11 @@ namespace __debug
typedef typename _Base::const_pointer const_pointer;
// 23.2.3.1 construct/copy/destroy:
+
+ forward_list() = default;
+
explicit
- forward_list(const allocator_type& __al = allocator_type())
+ forward_list(const allocator_type& __al) noexcept
: _Base(__al) { }
forward_list(const forward_list& __list, const allocator_type& __al)
diff --git a/libstdc++-v3/include/profile/forward_list b/libstdc++-v3/include/profile/forward_list
index 295c4a5..ab7ed04 100644
--- a/libstdc++-v3/include/profile/forward_list
+++ b/libstdc++-v3/include/profile/forward_list
@@ -51,8 +51,11 @@ namespace __profile
typedef typename _Base::const_iterator const_iterator;
// 23.2.3.1 construct/copy/destroy:
+
+ forward_list() = default;
+
explicit
- forward_list(const _Alloc& __al = _Alloc())
+ forward_list(const _Alloc& __al) noexcept
: _Base(__al) { }
forward_list(const forward_list& __list, const _Alloc& __al)