Re: _FORTIFY_SOURCE for std::vector

[Florian Weimer <fweimer at redhat dot com>]

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.)

--
Florian Weimer / Red Hat Product Security Team

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;
+}