diff mbox

libstdc++/61086 - fix ubsan errors in std::vector

Message ID 20140507120743.GB18109@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely May 7, 2014, 12:07 p.m. UTC 
The testcase in the PR calls __position._M_const_cast() to get a
mutable iterator and that dereferences the pointer as suggested in
http://gcc.gnu.org/ml/libstdc++/2013-05/msg00031.html
That's invalid because the pointer is not dereferenceable (in this
case it's null but is past-the-end at all times).

I played around with changing the __normal_iterator so we would do
__postition._M_const_cast(begin()) then decided we don't need it at
all and can just as easily obtain a mutable iterator using:

   auto __pos = begin() + (__position - cbegin());

I plan to commit the attached patch to trunk and 4.9 soon. I've tested
it on x86_64-linux but not added a testcase because we don't test with
-fsanitize (though we should do) and it only shows up with Clang
anyway.

Comments

Paolo Carlini May 7, 2014, 12:21 p.m. UTC | #1 
On 05/07/2014 02:07 PM, Jonathan Wakely wrote:
> The testcase in the PR calls __position._M_const_cast() to get a
> mutable iterator and that dereferences the pointer as suggested in
> http://gcc.gnu.org/ml/libstdc++/2013-05/msg00031.html
> That's invalid because the pointer is not dereferenceable (in this
> case it's null but is past-the-end at all times).
Uhmm, I see, at the time I scratched my head a bit. Nice that we can 
avoid the whole thing. Are we sure we don't have something similar 
elsewhere?

Paolo.
Jonathan Wakely May 7, 2014, 12:33 p.m. UTC | #2 
On 07/05/14 14:21 +0200, Paolo Carlini wrote:
>
>On 05/07/2014 02:07 PM, Jonathan Wakely wrote:
>>The testcase in the PR calls __position._M_const_cast() to get a
>>mutable iterator and that dereferences the pointer as suggested in
>>http://gcc.gnu.org/ml/libstdc++/2013-05/msg00031.html
>>That's invalid because the pointer is not dereferenceable (in this
>>case it's null but is past-the-end at all times).
>Uhmm, I see, at the time I scratched my head a bit. Nice that we can 
>avoid the whole thing. Are we sure we don't have something similar 
>elsewhere?

Yes, I checked. deque::const_iterator, list::const_iterator,
vector<bool>::const_iterator and the _Rb_tree_const_iterator types all
have _M_const_cast but they do not dereference anything.

It only really affected std::vector because that's the only one of our
containers that correctly supports custom pointer types (when my fixes
for PR57272 are ready I'll need to deal with the issue again and will
be careful about dereferencing).
Paolo Carlini May 7, 2014, 12:51 p.m. UTC | #3 
Hi,

On 05/07/2014 02:33 PM, Jonathan Wakely wrote:
> Yes, I checked. deque::const_iterator, list::const_iterator,
> vector<bool>::const_iterator and the _Rb_tree_const_iterator types all 
> have _M_const_cast but they do not dereference anything.
>
> It only really affected std::vector because that's the only one of our
> containers that correctly supports custom pointer types (when my fixes 
> for PR57272 are ready I'll need to deal with the issue again and will 
> be careful about dereferencing).
Excellent. Thanks again!

Paolo.
diff mbox

Patch

commit 566623def309c70387e41da2346ff89aa7619b13
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed May 7 12:17:41 2014 +0100

    	PR libstdc++/61086
    	* include/bits/stl_iterator.h (__normal_iterator::_M_const_cast):
    	Remove.
    	* include/bits/stl_vector.h (vector::insert, vector::erase): Use
    	arithmetic to obtain a mutable iterator from const_iterator.
    	* include/bits/vector.tcc (vector::insert): Likewise.
    	* include/debug/vector (vector::erase): Likewise.
    	* testsuite/23_containers/vector/requirements/dr438/assign_neg.cc:
    	Adjust dg-error line number.
    	* testsuite/23_containers/vector/requirements/dr438/
    	constructor_1_neg.cc: Likewise.
    	* testsuite/23_containers/vector/requirements/dr438/
    	constructor_2_neg.cc: Likewise.
    	* testsuite/23_containers/vector/requirements/dr438/insert_neg.cc:
    	Likewise.

diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
index 16f992c..f4522a4 100644
--- a/libstdc++-v3/include/bits/stl_iterator.h
+++ b/libstdc++-v3/include/bits/stl_iterator.h
@@ -736,21 +736,6 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		      _Container>::__type>& __i) _GLIBCXX_NOEXCEPT
         : _M_current(__i.base()) { }
 
-#if __cplusplus >= 201103L
-      __normal_iterator<typename _Container::pointer, _Container>
-      _M_const_cast() const noexcept
-      {
-	using _PTraits = std::pointer_traits<typename _Container::pointer>;
-	return __normal_iterator<typename _Container::pointer, _Container>
-	  (_PTraits::pointer_to(const_cast<typename _PTraits::element_type&>
-				(*_M_current)));
-      }
-#else
-      __normal_iterator
-      _M_const_cast() const
-      { return *this; }
-#endif
-
       // Forward iterator requirements
       reference
       operator*() const _GLIBCXX_NOEXCEPT
diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index 3d3a2cf..0a56c65 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -1051,7 +1051,7 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       insert(const_iterator __position, size_type __n, const value_type& __x)
       {
 	difference_type __offset = __position - cbegin();
-	_M_fill_insert(__position._M_const_cast(), __n, __x);
+	_M_fill_insert(begin() + __offset, __n, __x);
 	return begin() + __offset;
       }
 #else
@@ -1096,7 +1096,7 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 	       _InputIterator __last)
         {
 	  difference_type __offset = __position - cbegin();
-	  _M_insert_dispatch(__position._M_const_cast(),
+	  _M_insert_dispatch(begin() + __offset,
 			     __first, __last, __false_type());
 	  return begin() + __offset;
 	}
@@ -1144,10 +1144,11 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       iterator
 #if __cplusplus >= 201103L
       erase(const_iterator __position)
+      { return _M_erase(begin() + (__position - cbegin())); }
 #else
       erase(iterator __position)
+      { return _M_erase(__position); }
 #endif
-      { return _M_erase(__position._M_const_cast()); }
 
       /**
        *  @brief  Remove a range of elements.
@@ -1170,10 +1171,15 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       iterator
 #if __cplusplus >= 201103L
       erase(const_iterator __first, const_iterator __last)
+      {
+	const auto __beg = begin();
+	const auto __cbeg = cbegin();
+	return _M_erase(__beg + (__first - __cbeg), __beg + (__last - __cbeg));
+      }
 #else
       erase(iterator __first, iterator __last)
+      { return _M_erase(__first, __last); }
 #endif
-      { return _M_erase(__first._M_const_cast(), __last._M_const_cast()); }
 
       /**
        *  @brief  Swaps data with another %vector.
diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
index 299e614..5c3dfae 100644
--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -121,14 +121,17 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       else
 	{
 #if __cplusplus >= 201103L
+	  const auto __pos = begin() + (__position - cbegin());
 	  if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage)
 	    {
 	      _Tp __x_copy = __x;
-	      _M_insert_aux(__position._M_const_cast(), std::move(__x_copy));
+	      _M_insert_aux(__pos, std::move(__x_copy));
 	    }
 	  else
+	    _M_insert_aux(__pos, __x);
+#else
+	    _M_insert_aux(__position, __x);
 #endif
-	    _M_insert_aux(__position._M_const_cast(), __x);
 	}
       return iterator(this->_M_impl._M_start + __n);
     }
@@ -307,7 +310,7 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 	    ++this->_M_impl._M_finish;
 	  }
 	else
-	  _M_insert_aux(__position._M_const_cast(),
+	  _M_insert_aux(begin() + (__position - cbegin()),
 			std::forward<_Args>(__args)...);
 	return iterator(this->_M_impl._M_start + __n);
       }
diff --git a/libstdc++-v3/include/debug/vector b/libstdc++-v3/include/debug/vector
index 427d19f..f92a77f 100644
--- a/libstdc++-v3/include/debug/vector
+++ b/libstdc++-v3/include/debug/vector
@@ -647,7 +647,7 @@  namespace __debug
 	  }
 	else
 #if __cplusplus >= 201103L
-	  return iterator(__first.base()._M_const_cast(), this);
+	  return begin() + (__first.base() - cbegin().base());
 #else
 	  return __first;
 #endif
diff --git a/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/assign_neg.cc b/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/assign_neg.cc
index 191fbc7..655c7e8 100644
--- a/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/assign_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/assign_neg.cc
@@ -18,7 +18,7 @@ 
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1320 }
+// { dg-error "no matching" "" { target *-*-* } 1326 }
 
 #include <vector>
 
diff --git a/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc b/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc
index 8818a88..d938aa2 100644
--- a/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc
@@ -18,7 +18,7 @@ 
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1246 }
+// { dg-error "no matching" "" { target *-*-* } 1252 }
 
 #include <vector>
 
diff --git a/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc b/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc
index 09499bc..f888285 100644
--- a/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc
@@ -18,7 +18,7 @@ 
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1246 }
+// { dg-error "no matching" "" { target *-*-* } 1252 }
 
 #include <vector>
 #include <utility>
diff --git a/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/insert_neg.cc b/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/insert_neg.cc
index 674e3b5..316249b 100644
--- a/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/insert_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/insert_neg.cc
@@ -18,7 +18,7 @@ 
 // <http://www.gnu.org/licenses/>.
 
 // { dg-do compile }
-// { dg-error "no matching" "" { target *-*-* } 1361 }
+// { dg-error "no matching" "" { target *-*-* } 1367 }
 
 #include <vector>