[patch] libstdc++/65352 fix ubsan errors in std::array<T, 0>

[patch] libstdc++/65352 fix ubsan errors in std::array<T, 0>

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 232 bytes --]

Unsurprisingly ubsan doesn't like referencing a null pointer.

With this change __array_traits::_S_ref is only used to access an
element, which is invalid for std::array<T, 0> anyway.

Tested powerpc64le-linux, committed to trunk.


[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 3069 bytes --]

commit 0d999cf16b8f6a0d9bbf4bfe96b29e7b73a259e4
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu May 28 12:21:36 2015 +0100

    	PR libstdc++/65352
    	* include/std/array (__array_traits::_S_ptr): New function.
    	(array::data): Use _S_ptr to avoid creating invalid reference.
    	* testsuite/23_containers/array/tuple_interface/get_neg.cc: Adjust
    	dg-error line numbers.
    	* testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc:
    	likewise.

diff --git a/libstdc++-v3/include/std/array b/libstdc++-v3/include/std/array
index 429506b..24be44f 100644
--- a/libstdc++-v3/include/std/array
+++ b/libstdc++-v3/include/std/array
@@ -51,6 +51,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       static constexpr _Tp&
       _S_ref(const _Type& __t, std::size_t __n) noexcept
       { return const_cast<_Tp&>(__t[__n]); }
+
+      static constexpr _Tp*
+      _S_ptr(const _Type& __t) noexcept
+      { return const_cast<_Tp*>(__t); }
     };
 
  template<typename _Tp>
@@ -61,6 +65,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
      static constexpr _Tp&
      _S_ref(const _Type&, std::size_t) noexcept
      { return *static_cast<_Tp*>(nullptr); }
+
+     static constexpr _Tp*
+     _S_ptr(const _Type&) noexcept
+     { return nullptr; }
    };
 
   /**
@@ -219,11 +227,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 
       pointer
       data() noexcept
-      { return std::__addressof(_AT_Type::_S_ref(_M_elems, 0)); }
+      { return _AT_Type::_S_ptr(_M_elems); }
 
       const_pointer
       data() const noexcept
-      { return std::__addressof(_AT_Type::_S_ref(_M_elems, 0)); }
+      { return _AT_Type::_S_ptr(_M_elems); }
     };
 
   // Array comparisons.
diff --git a/libstdc++-v3/testsuite/23_containers/array/tuple_interface/get_neg.cc b/libstdc++-v3/testsuite/23_containers/array/tuple_interface/get_neg.cc
index 7604412..6830964 100644
--- a/libstdc++-v3/testsuite/23_containers/array/tuple_interface/get_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/array/tuple_interface/get_neg.cc
@@ -28,6 +28,6 @@ int n1 = std::get<1>(a);
 int n2 = std::get<1>(std::move(a));
 int n3 = std::get<1>(ca);
 
-// { dg-error "static assertion failed" "" { target *-*-* } 274 }
-// { dg-error "static assertion failed" "" { target *-*-* } 283 }
+// { dg-error "static assertion failed" "" { target *-*-* } 282 }
 // { dg-error "static assertion failed" "" { target *-*-* } 291 }
+// { dg-error "static assertion failed" "" { target *-*-* } 299 }
diff --git a/libstdc++-v3/testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc b/libstdc++-v3/testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc
index 9788053..5d75366 100644
--- a/libstdc++-v3/testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc
@@ -23,4 +23,4 @@
 
 typedef std::tuple_element<1, std::array<int, 1>>::type type;
 
-// { dg-error "static assertion failed" "" { target *-*-* } 322 }
+// { dg-error "static assertion failed" "" { target *-*-* } 330 }

Re: [patch] libstdc++/65352 fix ubsan errors in std::array<T, 0>

[Jonathan Wakely]

On 28/05/15 12:53 +0100, Jonathan Wakely wrote:
>Unsurprisingly ubsan doesn't like referencing a null pointer.
>
>With this change __array_traits::_S_ref is only used to access an
>element, which is invalid for std::array<T, 0> anyway.
>
>Tested powerpc64le-linux, committed to trunk.

And gcc-5-branch.

>commit 0d999cf16b8f6a0d9bbf4bfe96b29e7b73a259e4
>Author: Jonathan Wakely <jwakely@redhat.com>
>Date:   Thu May 28 12:21:36 2015 +0100
>
>    	PR libstdc++/65352
>    	* include/std/array (__array_traits::_S_ptr): New function.
>    	(array::data): Use _S_ptr to avoid creating invalid reference.
>    	* testsuite/23_containers/array/tuple_interface/get_neg.cc: Adjust
>    	dg-error line numbers.
>    	* testsuite/23_containers/array/tuple_interface/tuple_element_neg.cc:
>    	likewise.

Re: [patch] libstdc++/65352 fix ubsan errors in std::array<T, 0>

[Marc Glisse]

On Thu, 28 May 2015, Jonathan Wakely wrote:

> Unsurprisingly ubsan doesn't like referencing a null pointer.
>
> With this change __array_traits::_S_ref is only used to access an
> element, which is invalid for std::array<T, 0> anyway.

Should

return *static_cast<_Tp*>(nullptr);

be replaced with

__builtin_unreachable();

then? It seems strange to keep an implementation that is never supposed to 
be used.

-- 
Marc Glisse

Re: [patch] libstdc++/65352 fix ubsan errors in std::array<T, 0>

[Jonathan Wakely]

On 28/05/15 15:26 +0200, Marc Glisse wrote:
>On Thu, 28 May 2015, Jonathan Wakely wrote:
>
>>Unsurprisingly ubsan doesn't like referencing a null pointer.
>>
>>With this change __array_traits::_S_ref is only used to access an
>>element, which is invalid for std::array<T, 0> anyway.
>
>Should
>
>return *static_cast<_Tp*>(nullptr);
>
>be replaced with
>
>__builtin_unreachable();
>
>then? It seems strange to keep an implementation that is never 
>supposed to be used.

That's a good idea, I experimented with just not defining it but that
fails for explicit instantiations of array<T, 0>.

Re: [patch] libstdc++/65352 fix ubsan errors in std::array<T, 0>

[Jonathan Wakely]

On 28/05/15 14:38 +0100, Jonathan Wakely wrote:
>On 28/05/15 15:26 +0200, Marc Glisse wrote:
>>On Thu, 28 May 2015, Jonathan Wakely wrote:
>>
>>>Unsurprisingly ubsan doesn't like referencing a null pointer.
>>>
>>>With this change __array_traits::_S_ref is only used to access an
>>>element, which is invalid for std::array<T, 0> anyway.
>>
>>Should
>>
>>return *static_cast<_Tp*>(nullptr);
>>
>>be replaced with
>>
>>__builtin_unreachable();
>>
>>then? It seems strange to keep an implementation that is never 
>>supposed to be used.
>
>That's a good idea, I experimented with just not defining it but that
>fails for explicit instantiations of array<T, 0>.

Would there be a danger of an object compiled with gcc-5.1 that calls
array<T, 0>::data() finding the _S_ref from an object compiled with
gcc-5.2 and hitting the __builtin_unreachable in vali code?

Re: [patch] libstdc++/65352 fix ubsan errors in std::array<T, 0>

[Marc Glisse]

On Thu, 28 May 2015, Jonathan Wakely wrote:

> Would there be a danger of an object compiled with gcc-5.1 that calls
> array<T, 0>::data() finding the _S_ref from an object compiled with
> gcc-5.2 and hitting the __builtin_unreachable in vali code?

At -O0, maybe. To be safe you would need to give this _S_ref an arbitrary 
abi_tag. You could also replace all uses of _S_ref with *_S_ptr.

-- 
Marc Glisse

Re: [patch] libstdc++/65352 fix ubsan errors in std::array<T, 0>

[Jonathan Wakely]

On 28/05/15 15:52 +0200, Marc Glisse wrote:
>On Thu, 28 May 2015, Jonathan Wakely wrote:
>
>>Would there be a danger of an object compiled with gcc-5.1 that calls
>>array<T, 0>::data() finding the _S_ref from an object compiled with
>>gcc-5.2 and hitting the __builtin_unreachable in vali code?
>
>At -O0, maybe. To be safe you would need to give this _S_ref an 
>arbitrary abi_tag.

Or just rename it.

>You could also replace all uses of _S_ref with *_S_ptr.

I considered this, but I thought that changing _S_ref(_M_elems, n) to
_S_ptr(_M_elems)[n] would fail to give an error for out-of-range
accesses in constant expressions e.g.

    constexpr std::array<int, 1> a{};
    constexpr int i = a[1];

But it still seems to give an error, so maybe getting rid of _S_ref
entirely is the way to go.

Re: [patch] libstdc++/65352 fix ubsan errors in std::array<T, 0>

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

On 28/05/15 12:53 +0100, Jonathan Wakely wrote:
>Unsurprisingly ubsan doesn't like referencing a null pointer.
>
>With this change __array_traits::_S_ref is only used to access an
>element, which is invalid for std::array<T, 0> anyway.
>
>Tested powerpc64le-linux, committed to trunk.

I forgot the debug and profile modes, fixed like so.

1) Why do we even have _profile::array? What's it for?

2) If we could run 'make check-sanitize' I could have added tests for
   this bug, and could have found it still failed in debug and profile
   modes. We need to be able to run the testsuite with ubsan.

I'll commit it to trunk and gcc-5-branch after testing.


[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 1440 bytes --]

commit 7a673c403d77fb2c57620f5e4f027b679bf69635
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu May 28 15:35:43 2015 +0100

    	PR libstdc++/65352
    	* include/profile/array (array::data): Use _S_ptr.
    	* include/debug/array (array::data): Likewise.

diff --git a/libstdc++-v3/include/debug/array b/libstdc++-v3/include/debug/array
index 31d146e..411e816 100644
--- a/libstdc++-v3/include/debug/array
+++ b/libstdc++-v3/include/debug/array
@@ -216,11 +216,11 @@ namespace __debug
 
       pointer
       data() noexcept
-      { return std::__addressof(_AT_Type::_S_ref(_M_elems, 0)); }
+      { return _AT_Type::_S_ptr(_M_elems); }
 
       const_pointer
       data() const noexcept
-      { return std::__addressof(_AT_Type::_S_ref(_M_elems, 0)); }
+      { return _AT_Type::_S_ptr(_M_elems); }
     };
 
   // Array comparisons.
diff --git a/libstdc++-v3/include/profile/array b/libstdc++-v3/include/profile/array
index a90e396..5198bb3 100644
--- a/libstdc++-v3/include/profile/array
+++ b/libstdc++-v3/include/profile/array
@@ -178,11 +178,11 @@ namespace __profile
 
       pointer
       data() noexcept
-      { return std::__addressof(_AT_Type::_S_ref(_M_elems, 0)); }
+      { return _AT_Type::_S_ptr(_M_elems); }
 
       const_pointer
       data() const noexcept
-      { return std::__addressof(_AT_Type::_S_ref(_M_elems, 0)); }
+      { return _AT_Type::_S_ptr(_M_elems); }
     };
 
   // Array comparisons.