Patchwork _FORTIFY_SOURCE for std::vector

login
register
mail settings
Submitter Florian Weimer
Date June 4, 2012, 5:54 p.m.
Message ID <4FCCF671.6000403@redhat.com>
Download mbox | patch
Permalink /patch/162846/
State New
Headers show

Comments

Florian Weimer - June 4, 2012, 5:54 p.m.
On 06/01/2012 01:34 PM, Jakub Jelinek wrote:
> Have you looked at the assembly differences with this in?

It's not great.

Here's an example:

void
write(std::vector<float>& blob, unsigned n, float v1, float v2, float 
v3, float v4)
{
   blob[n] = v1;
   blob[n + 1] = v2;
   blob[n + 2] = v3;
   blob[n + 3] = v4;
}

Without checks, it turns into:

	movq	(%rdi), %rax
	movl	%esi, %edx
	movss	%xmm0, (%rax,%rdx,4)
	leal	1(%rsi), %edx
	movss	%xmm1, (%rax,%rdx,4)
	leal	2(%rsi), %edx
	movss	%xmm2, (%rax,%rdx,4)
	leal	3(%rsi), %edx
	movss	%xmm3, (%rax,%rdx,4)
	ret

With checks, it's:

	subq	$8, %rsp
	movq	(%rdi), %rdx
	movq	8(%rdi), %rax
	movl	%esi, %ecx
	subq	%rdx, %rax
	sarq	$2, %rax
	cmpq	%rax, %rcx
	jae	.L8
	movss	%xmm0, (%rdx,%rcx,4)
	leal	1(%rsi), %ecx
	cmpq	%rax, %rcx
	jae	.L8
	movss	%xmm1, (%rdx,%rcx,4)
	leal	2(%rsi), %ecx
	cmpq	%rax, %rcx
	jae	.L8
	movss	%xmm2, (%rdx,%rcx,4)
	leal	3(%rsi), %ecx
	cmpq	%rax, %rcx
	jae	.L8
	movss	%xmm3, (%rdx,%rcx,4)
	addq	$8, %rsp
	ret
.L8:
	call	__chk_fail

For char/signed char/unsigned char, it's even worse because there's an 
additional pointer load per store (bringing up the total to two—the 
compiler cannot see that the _M_start pointer cannot point into the 
std::vector object).

I'm no longer sure that's acceptable overhead.  We really don't want 
programmers to switch off _FORTIFY_SOURCE because of its overhead.

I'm attaching the patch just in case.  I'm not sure the test case 
actually ran (if it did, it passed on first try, which would be unusual.)
Marc Glisse - June 4, 2012, 7:07 p.m.
On Mon, 4 Jun 2012, Florian Weimer wrote:

> On 06/01/2012 01:34 PM, Jakub Jelinek wrote:
>> Have you looked at the assembly differences with this in?
>
> It's not great.
>
> Here's an example:
>
> void
> write(std::vector<float>& blob, unsigned n, float v1, float v2, float v3, 
> float v4)
> {
>  blob[n] = v1;
>  blob[n + 1] = v2;
>  blob[n + 2] = v3;
>  blob[n + 3] = v4;
> }

Would be great if it ended up testing only n and n+3.
__attribute__((__noreturn__)) is not quite strong enough to allow this 
optimization, it would require something like 
__attribute__((__crashing__)) to let the compiler know that if the 
function is called, you don't care what happens to blob. And possibly the 
use of a signed n.

Note that even when the optimization would be legal, gcc seems to have a 
few difficulties:

extern "C" void fail() __attribute((noreturn));
void write(signed m, signed n)
{
   if((n+3)>m) fail();
   if((n+2)>m) fail();
   if((n+1)>m) fail();
   if(n>m) fail();
}

keeps 3 tests.
Florian Weimer - June 4, 2012, 7:35 p.m.
On 06/04/2012 09:07 PM, Marc Glisse wrote:
> On Mon, 4 Jun 2012, Florian Weimer wrote:
>> void
>> write(std::vector<float>& blob, unsigned n, float v1, float v2, float
>> v3, float v4)
>> {
>> blob[n] = v1;
>> blob[n + 1] = v2;
>> blob[n + 2] = v3;
>> blob[n + 3] = v4;
>> }
>
> Would be great if it ended up testing only n and n+3.

True.

> __attribute__((__noreturn__)) is not quite strong enough to allow this
> optimization, it would require something like
> __attribute__((__crashing__)) to let the compiler know that if the
> function is called, you don't care what happens to blob. And possibly
> the use of a signed n.

Interesting point, I had not realized that before.  Ada has a special 
rule for failures of language-defined checks, and they might give enough 
wiggle room to leave behind a partially updated vector in such situations.

But even without that, you could clone the if sequence, that is,

   if (blob.size() - n >= 4)
     {
       blob[n] = v1;
       blob[n + 1] = v2;
       blob[n + 2] = v3;
       blob[n + 3] = v4;
     }
   else
     {
        ... // individual checks
     }

Obviously, this has quite a bit of an impact on code size.
Richard Guenther - June 5, 2012, 9:05 a.m.
On Mon, Jun 4, 2012 at 9:07 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Mon, 4 Jun 2012, Florian Weimer wrote:
>
>> On 06/01/2012 01:34 PM, Jakub Jelinek wrote:
>>>
>>> Have you looked at the assembly differences with this in?
>>
>>
>> It's not great.
>>
>> Here's an example:
>>
>> void
>> write(std::vector<float>& blob, unsigned n, float v1, float v2, float v3,
>> float v4)
>> {
>>  blob[n] = v1;
>>  blob[n + 1] = v2;
>>  blob[n + 2] = v3;
>>  blob[n + 3] = v4;
>> }
>
>
> Would be great if it ended up testing only n and n+3.
> __attribute__((__noreturn__)) is not quite strong enough to allow this
> optimization, it would require something like __attribute__((__crashing__))
> to let the compiler know that if the function is called, you don't care what
> happens to blob. And possibly the use of a signed n.
>
> Note that even when the optimization would be legal, gcc seems to have a few
> difficulties:
>
> extern "C" void fail() __attribute((noreturn));
> void write(signed m, signed n)
> {
>  if((n+3)>m) fail();
>  if((n+2)>m) fail();
>  if((n+1)>m) fail();
>  if(n>m) fail();
> }
>
> keeps 3 tests.

Well, the issue is that we'd first need to commonize the fail () calls which we
do now, but even then VRP fails to simplify the comparisons against the
symbolic ranges (it's not very good at that).

And that would only be at -O1.  Note that such range-checks will defeat
most, if not all, loop optimizations, too.  So C++ code using std::vector
in compute-intensive parts would be severely pessimized.

So, I don't think fortifying libstdc++ is a good idea at all.

Richard.

> --
> Marc Glisse
Florian Weimer - June 6, 2012, 12:22 p.m.
On 06/05/2012 11:05 AM, Richard Guenther wrote:
> And that would only be at -O1.  Note that such range-checks will defeat
> most, if not all, loop optimizations, too.  So C++ code using std::vector
> in compute-intensive parts would be severely pessimized.

Array bounds check elimination could deal with that, but that would 
require to teach the compiler about std::vector internals.

> So, I don't think fortifying libstdc++ is a good idea at all.

For std::vector and operator[] I agree, but I wouldn't discard the 
entire idea completely.  (std::array would be much cheaper to check, but 
then we'd lose consistency.)

I'm going to look for tool support for locating such uses of operator[]. 
  We might start recommending developers to switch to the at() member 
functions in most code.

Patch

Index: libstdc++-v3/include/bits/c++config
===================================================================
--- libstdc++-v3/include/bits/c++config	(revision 187951)
+++ libstdc++-v3/include/bits/c++config	(working copy)
@@ -370,6 +370,13 @@ 
   } while (false)
 #endif
 
+// _FORTIFY_SOURCE support.  Only available on GNU libc.
+
+namespace __gnu_cxx
+{
+  extern "C" void __chk_fail () __attribute__((noreturn));
+}
+
 // Macros for race detectors.
 // _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(A) and
 // _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(A) should be used to explain
Index: libstdc++-v3/include/bits/stl_vector.h
===================================================================
--- libstdc++-v3/include/bits/stl_vector.h	(revision 187951)
+++ libstdc++-v3/include/bits/stl_vector.h	(working copy)
@@ -768,7 +768,10 @@ 
        */
       reference
       operator[](size_type __n)
-      { return *(this->_M_impl._M_start + __n); }
+      {
+	_M_fortify_range_check(__n);
+	return *(this->_M_impl._M_start + __n);
+      }
 
       /**
        *  @brief  Subscript access to the data contained in the %vector.
@@ -783,7 +786,10 @@ 
        */
       const_reference
       operator[](size_type __n) const
-      { return *(this->_M_impl._M_start + __n); }
+      { 
+	_M_fortify_range_check(__n);
+	return *(this->_M_impl._M_start + __n);
+      }
 
     protected:
       /// Safety check used only from at().
@@ -794,6 +800,17 @@ 
 	  __throw_out_of_range(__N("vector::_M_range_check"));
       }
 
+      /// Range check used by operator[].
+      /// No-op unless _FORTIFY_SOURCE is enabled.
+      void
+      _M_fortify_range_check(size_type __n) const
+      {
+#if defined __USE_FORTIFY_LEVEL && __USE_FORTIFY_LEVEL > 0
+	if (__n >= this->size())
+	    __gnu_cxx::__chk_fail();
+#endif
+      }
+
     public:
       /**
        *  @brief  Provides access to the data contained in the %vector.
Index: libstdc++-v3/testsuite/23_containers/vector/element_access/2.cc
===================================================================
--- libstdc++-v3/testsuite/23_containers/vector/element_access/2.cc	(revision 0)
+++ libstdc++-v3/testsuite/23_containers/vector/element_access/2.cc	(revision 0)
@@ -0,0 +1,100 @@ 
+// Copyright (C) 2012 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/>.
+
+// 23.2.4 vector 
+
+// { dg-add-options no_pch }
+// { dg-target *-*-linux* }
+
+#undef _FORTIFY_SOURCE
+#define _FORTIFY_SOURCE 2
+
+#include <vector>
+
+#include <fcntl.h>
+#include <paths.h>
+#include <setjmp.h>
+#include <signal.h>
+
+#include <testsuite_hooks.h>
+
+// Testing pattern follows debug/test-strcpy_chk.c in GNU libc.
+volatile int chk_fail_ok;
+jmp_buf chk_fail_buf;
+
+static void
+handler(int)
+{
+  if (chk_fail_ok)
+    {
+      chk_fail_ok = 0;
+      longjmp(chk_fail_buf, 1);
+    }
+  else
+    _exit(127);
+}
+
+static void
+run_test(void (*t)())
+{
+  bool test __attribute__((unused)) = true;
+  if (setjmp(chk_fail_buf) == 0)
+    {
+      t();
+      VERIFY(false);
+    }
+}
+
+void test01()
+{
+  std::vector<int> v(5);
+  v[0];
+  v[4];
+  chk_fail_ok = 1;
+  v[5];
+}
+
+void test02()
+{
+  std::vector<int> u(5);
+  const std::vector<int>& v(u);
+  v[0];
+  v[4];
+  chk_fail_ok = 1;
+  v[5];
+}
+
+int main()
+{
+  // See debug/test-strcpy_chk.c in GNU libc.
+  struct sigaction sa;
+  sa.sa_handler = andler;
+  sa.sa_flags = 0;
+  sigaction(SIGABRT, &sa, NULL);
+  int fd = open(_PATH_DEVNULL, O_WRONLY);
+  if (fd == -1)
+    close(STDERR_FILENO);
+  else
+    {
+      dup2(fd, STDERR_FILENO);
+      close(fd);
+    }
+  setenv("LIBC_FATAL_STDERR_", "1", 1); // do not use /dev/tty
+  run_test(test01);
+  run_test(test02);
+  return 0;
+}