diff mbox

proposed fix for libstdc++/59829

Message ID 20140129145918.GB21297@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely Jan. 29, 2014, 2:59 p.m. UTC 
On 27/01/14 23:37 +0000, Jonathan Wakely wrote:
>On 27 January 2014 20:35, Jonathan Wakely wrote:
>> On 27 January 2014 20:12, Marc Glisse wrote:
>>> On Mon, 27 Jan 2014, Jonathan Wakely wrote:
>>>
>>>> This is the best I've come up with to avoid dereferencing an invalid
>>>> pointer when calling vector::data() on an empty vector.
>>>>
>>>> For C++03 we reurn the vector's pointer type, so can just return the
>>>> internal pointer, but for C++11 we need to convert that to a raw
>>>> pointer, which we do by dereferencing, so we must check if it's valid
>>>> first.
>>>
>>>
>>> For comparison, libc++ has 2 paths. If pointer really is a pointer, it just
>>> returns it, no need to pay for a comparison in that case. And otherwise, it
>>> calls _M_start.operator-> and crosses its fingers. There is a helper
>>> function doing that used throughout the library.
>>
>> Ah yes, I remember Howard posting a get_raw_pointer() function to the
>> reflector that used operator->() on user-defined types ... I don't
>> really like calling that on a potentially invalid pointer though. The
>> user-defined pointer type in my new testcase could just as easily
>> throw if operator-> is called on an invalid pointer.  As Paolo also
>> mentioned avoiding the branch for built-in pointers I'll do that.

Here's what I'm committing, the testcase is simplified by reusing the
new PointerBase type I added to testsuite_allocator.h

Tested x86_64-linux, committed to trunk.
commit 36805f8b1b6ef6733c4e018ef005efb1575b75c6
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jan 28 12:47:25 2014 +0000

    	PR libstdc++/59829
    	* include/bits/stl_vector.h (vector::data()): Call _M_data_ptr.
    	(vector::_M_data_ptr): New overloaded functions to ensure empty
    	vectors do not dereference the pointer.
    	* testsuite/23_containers/vector/59829.cc: New.
    	* 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 mbox

Patch

diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index 98ac708..7e52fde 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -888,7 +888,7 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       pointer
 #endif
       data() _GLIBCXX_NOEXCEPT
-      { return std::__addressof(front()); }
+      { return _M_data_ptr(this->_M_impl._M_start); }
 
 #if __cplusplus >= 201103L
       const _Tp*
@@ -896,7 +896,7 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       const_pointer
 #endif
       data() const _GLIBCXX_NOEXCEPT
-      { return std::__addressof(front()); }
+      { return _M_data_ptr(this->_M_impl._M_start); }
 
       // [23.2.4.3] modifiers
       /**
@@ -1470,6 +1470,23 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 	  }
       }
 #endif
+
+#if __cplusplus >= 201103L
+      template<typename _Up>
+	_Up*
+	_M_data_ptr(_Up* __ptr) const
+	{ return __ptr; }
+
+      template<typename _Ptr>
+	typename std::pointer_traits<_Ptr>::element_type*
+	_M_data_ptr(_Ptr __ptr) const
+	{ return empty() ? nullptr : std::__addressof(*__ptr); }
+#else
+      template<typename _Ptr>
+	_Ptr
+	_M_data_ptr(_Ptr __ptr) const
+	{ return __ptr; }
+#endif
     };
 
 
diff --git a/libstdc++-v3/testsuite/23_containers/vector/59829.cc b/libstdc++-v3/testsuite/23_containers/vector/59829.cc
new file mode 100644
index 0000000..1818c89
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/vector/59829.cc
@@ -0,0 +1,67 @@ 
+// Copyright (C) 2014 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
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-std=gnu++11" }
+
+// libstdc++/59829
+
+#include <vector>
+#include <testsuite_allocator.h>
+
+// User-defined pointer type that throws if a null pointer is dereferenced.
+template<typename T>
+struct Pointer : __gnu_test::PointerBase<Pointer<T>, T>
+{
+  using __gnu_test::PointerBase<Pointer<T>, T>::PointerBase;
+
+  T& operator*() const
+  {
+    if (!this->value)
+      throw "Dereferenced invalid pointer";
+    return *this->value;
+  }
+};
+
+// Minimal allocator using Pointer<T>
+template<typename T>
+struct Alloc
+{
+  typedef T value_type;
+  typedef Pointer<T> pointer;
+
+  Alloc() = default;
+  template<typename U>
+    Alloc(const Alloc<U>&) { }
+
+  pointer allocate(std::size_t n)
+  { return pointer(std::allocator<T>().allocate(n)); }
+
+  void deallocate(pointer p, std::size_t n)
+  { std::allocator<T>().deallocate(p.value, n); }
+};
+
+template<typename T>
+bool operator==(Alloc<T> l, Alloc<T> r) { return true; }
+
+template<typename T>
+bool operator!=(Alloc<T> l, Alloc<T> r) { return false; }
+
+int main()
+{
+  std::vector<int, Alloc<int>> a;
+  a.data();
+}
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 a12b116..191fbc7 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 *-*-* } 1316 }
+// { dg-error "no matching" "" { target *-*-* } 1320 }
 
 #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 b839ccc..8818a88 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 *-*-* } 1242 }
+// { dg-error "no matching" "" { target *-*-* } 1246 }
 
 #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 e9e966b..09499bc 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 *-*-* } 1242 }
+// { dg-error "no matching" "" { target *-*-* } 1246 }
 
 #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 71c6c49..674e3b5 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 *-*-* } 1357 }
+// { dg-error "no matching" "" { target *-*-* } 1361 }
 
 #include <vector>