diff mbox

proposed fix for libstdc++/59829

Message ID 20140127193851.GA16809@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely Jan. 27, 2014, 7:38 p.m. UTC
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.

Any better suggestions before I commit this?
(Tests pass, although I need to adjust some dg-error line numbers
before committing it.)
commit 60f9bb3b06ea95054d3fb442c9c212e7afe66acc
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon Jan 27 18:35:17 2014 +0000

    	PR libstdc++/59829
    	* include/bits/stl_vector.h (vector::data()): Do not dereference
    	pointers when empty.
    	* testsuite/23_containers/vector/59829.cc: New.

Comments

Marc Glisse Jan. 27, 2014, 8:12 p.m. UTC | #1
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.
Paolo Carlini Jan. 27, 2014, 8:17 p.m. UTC | #2
Hi,

> On 27/gen/2014, at 20:38, Jonathan Wakely <jwakely@redhat.com> 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.
> 
> Any better suggestions before I commit this?

I'm going to say something quite obvious - in principle the C++11 version could become identical at compile time to the simpler C++03 version depending on the types. Not sure if it's worth it, avoiding the conditional operator at run-time... Maybe libc++ does something similar, if I remember correctly Howard's distaste for branches ;)

Paolo
Jonathan Wakely Jan. 27, 2014, 8:35 p.m. UTC | #3
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.

(Sorry for the double-post, the first one seemed to disappear and I
wasn't sure if it really happened or not, so wrote it again!)
diff mbox

Patch

diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index f482957..8393242 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -880,19 +880,21 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        */
 #if __cplusplus >= 201103L
       _Tp*
-#else
-      pointer
-#endif
       data() _GLIBCXX_NOEXCEPT
-      { return std::__addressof(front()); }
+      { return empty() ? nullptr : std::__addressof(*this->_M_impl._M_start); }
 
-#if __cplusplus >= 201103L
       const _Tp*
+      data() const _GLIBCXX_NOEXCEPT
+      { return empty() ? nullptr : std::__addressof(*this->_M_impl._M_start); }
 #else
+      pointer
+      data() _GLIBCXX_NOEXCEPT
+      { return this->_M_impl._M_start; }
+
       const_pointer
-#endif
       data() const _GLIBCXX_NOEXCEPT
-      { return std::__addressof(front()); }
+      { return this->_M_impl._M_start; }
+#endif
 
       // [23.2.4.3] modifiers
       /**
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..8d9f39d
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/vector/59829.cc
@@ -0,0 +1,108 @@ 
+// 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>
+
+// User-defined pointer type that throws if a null pointer is dereferenced.
+template<typename T>
+struct Pointer
+{
+  typedef T element_type;
+
+  // typedefs for iterator_traits
+  typedef T value_type;
+  typedef std::ptrdiff_t difference_type;
+  typedef std::random_access_iterator_tag iterator_category;
+  typedef Pointer pointer;
+  typedef T& reference;
+
+  T* value;
+
+  explicit Pointer(T* p = nullptr) : value(p) { }
+
+  template<typename U, typename Requires = decltype((T*)std::declval<U*>())>
+    Pointer(const Pointer<U>& p) : value(p.value) { }
+
+  T& operator*() const
+  {
+    if (!value)
+      throw nullptr;
+    return *value;
+  }
+
+  T* operator->() const { return value; }
+
+  Pointer& operator++() { ++value; return *this; }
+  Pointer operator++(int) { Pointer tmp(*this); ++value; return tmp; }
+  Pointer& operator--() { --value; return *this; }
+  Pointer operator--(int) { Pointer tmp(*this); --value; return tmp; }
+
+  Pointer& operator+=(difference_type n) { value += n; return *this; }
+  Pointer& operator-=(difference_type n) { value -= n; return *this; }
+
+  explicit operator bool() const { return value != nullptr; }
+
+  Pointer operator+(difference_type n) { Pointer p(*this); return p += n; }
+  Pointer operator-(difference_type n) { Pointer p(*this); return p -= n; }
+};
+
+template<typename T>
+std::ptrdiff_t operator-(Pointer<T> l, Pointer<T> r)
+{ return l.value - r.value; }
+
+template<typename T>
+bool operator==(Pointer<T> l, Pointer<T> r)
+{ return l.value == r.value; }
+
+template<typename T>
+bool operator!=(Pointer<T> l, Pointer<T> r)
+{ return l.value != r.value; }
+
+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();
+}