diff mbox

[build] Move ENABLE_EXECUTE_STACK to toplevel libgcc

Message ID yddvcwmrkaj.fsf@manam.CeBiTec.Uni-Bielefeld.DE
State New
Headers show

Commit Message

Rainer Orth June 3, 2011, 3:45 p.m. UTC
Paolo Bonzini <bonzini@gnu.org> writes:

>> Seems like a plan, but I didn't go in this direction since I couldn't
>> test anything like this.
>
> As long as you test the general configury on 1-2 platforms, it's not any
> less tested than what you have now.

Unfortunately, that's not true: my original patch just moved the
existing ENABLE_EXECUTE_STACK definitions to new headers in libgcc and
used them as is.  The revised one below consolidates them into two new
files, only one of them has been tested so far.

>> The various __enable_execute_stack
>> implementations differ in minor ways that would have to be autoconf'ed.
>
> Just select them by $host.

Ok, I've done that.

>> One other caveat: I don't know if I like grouping the configure support
>> for the enable-execute-stack.c variants together or would rather keep
>> all configuration for a single platform (or OS) together.  Probably a
>> matter of taste.
>
> In case it wasn't clear, I was proposing the latter.

I suppose you mean the former, i.e. grouping them together and not
setting enable_execute_stack in the existing host cases?

>> Another question: should we keep the variants in libgcc/config (like
>> your config/enable-execute-stack-mprotect.c) or at the toplevel (like
>> enable-execute-stack-empty.c)?  At the moment I see a mixture of files
>> at the libgcc toplevel and others in config.
>
> I would put them under config/ unless they are platform-independent.

Ok, done.

>> I'd thought about it, but refrained since HAVE_ENABLE_EXECUTE_STACK
>> affects only three cpus.  Currently, our documentation of libgcc
>> configury and macros used is close to non-existant.  That's probably for
>> someone who has implemented this stuff.
>
> True, OTOH HAVE_ENABLE_EXECUTE_STACK is a target macro, and those are well
> documented.  Just say that it has to be defined if libgcc provides a
> non-trivial implementation of __enable_execute_stack; it doesn't need to
> delve into how to hack libgcc.

Will do when I submit the final patch.  Right now, I've got a plan and a
draft where I'm looking for comments.

>>> you can make the above work, that would be great as an example of how to
>>> move stuff to the libgcc toplevel directory.
>>
>> I'll give it a try, but it might take over the weekend.
>
> Take your time, we're in stage1 now.

We had a public holiday yesterday :-)

So here we go: I'm including only the libgcc part of the patch, the rest
is unchanged.

The patch is only lightly tested so far, i.e. bootstrapped on
i386-pc-solaris2.8 and i386-pc-solaris2.11.

The existing variants (listed by the previous libgcc headers) are:

      alpha/osf5-lib.h		mprotect
      				addr & page, end + TRAMPOLINE_SIZE
      darwin-lib.h		mprotect
      				addr & page, end + TARGET_64BIT ? 48 : 40
      				rs6000_trampoline_size, cannot use
      				trampoline size which is a function call
      freebsd-lib.h		mprotect
      				addr, TRAMPOLINE_SIZE, why no & page?
      				check_enabling via sysctlbyname
      i386/mingw32-lib.h	VirtualProtect
      netbsd-lib.h		mprotect
      				addr & page, end + TRAMPOLINE_SIZE
      				pagesize via __sysctl
      openbsd-lib.h		mprotect
      				addr & page, end + TRAMPOLINE_SIZE
      sol2-lib.h		mprotect
      				addr & page, end + TRAMPOLINE_SIZE
      				check_enabling via sysconf

mingw32 is completely separate, while the others have much in common,
namely the use of mprotect to enable stack RWX permissions.

But there are also considerable differences:

* Except for Darwin, the code uses TRAMPOLINE_SIZE.  This only exists in
  the backend headers.  While it could be duplicated somewhere in the
  libgcc configury, I'd rather propose that gcc define
  __TRAMPOLINE_SIZE__ (in gcc/c-family/c-cppbuiltin.c (c_cpp_builtins)
  to avoid this.  On PowerPC Darwin, one cannot use TRAMPOLINE_SIZE
  definition right now since the macro calls the rs6000_trampoline_size
  function in rs6000/rs6000.c.  This would be solved nicely by
  __TRAMPOLINE_SIZE__.

* FreeBSD and Solaris use a check_enabling function to check if
  __enable_execute_stack needs to run at all.  Initially, I had
  enable-execute-stack-{freebsd, netbsd, sol2}.c, all including a common
  enable-execute-stack-mprotect.c to avoid code duplication.  I now
  consider that ugly and hard to read, and merged everything into a
  common enable-execute-stack-mprotect.c below.  Right now, the file
  uses __FreeBSD__, __NetBSD__, and __sun__ && __svr4__ to select the
  right code, but this could be autoconf'ed if desired.  There are a
  couple of FIXME comments in the code.

* NetBSD is extremely careful to avoid namespace pollution and uses
  private __sysctl (declared privately) to avoid using getpagesize.
  Either this is an issue, and we might do something similar on all
  platforms, or it's not and we should avoid special-casing NetBSD
  here.

* FreeBSD uses the unmodified address passed to __enable_execute_stack
  to call mprocted, while all others round both address and size to a
  pagesize boundary.  I cannot imagine that FreeBSD supports
  byte-granularity mprotect, so this seems an oversight.

* Windows32 calls abort when VirtualProtect fails.  With the exception
  of Darwin and NetBSD, all others call perror, which seems to be
  frowned upon.  We should be consistent here.

* gcc/config/i386/mingw32.h has

#ifdef IN_LIBGCC2
#include <windows.h>
#endif

  I strongly suspect that this is only to declare VirtualQuery and
  VirtualProtect and can go if ENABLE_EXECUTE_STACK is removed?

* Finally, __enable_execute_stack is only used on those NetBSD targets
  that actually define HAVE_ENABLE_EXECUTE_STACK, while OpenBSD uses it
  everywhere.  config.host could be simplyfied if NetBSD behaved the
  same.

As you can see, quite a can of worms, but I'm getting closer to a clean
solution.

	Rainer


2011-05-29  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	gcc:
	* libgcc2.c [L_enable_execute_stack]: Remove.

	libgcc:
	* enable-execute-stack-empty.c: New file.
	* config/enable-execute-stack-mprotect.c: New file.
	* config/i386/enable-execute-stack-mingw32.c: New file.
	* config.host (enable_execute_stack): New variable.
	Select appropriate variants.
	* configure.ac: Link enable-execute-stack.c to
	$enable_execute_stack.
	* configure: Regenerate.
	* Makefile.in (LIB2ADD): Add enable-execute-stack.c.
	(lib2funcs): Remove _enable_execute_stack.

Comments

Paolo Bonzini June 3, 2011, 4:01 p.m. UTC | #1
On 06/03/2011 05:45 PM, Rainer Orth wrote:
> Paolo Bonzini<bonzini@gnu.org>  writes:
>
>>> >>  Seems like a plan, but I didn't go in this direction since I couldn't
>>> >>  test anything like this.
>> >
>> >  As long as you test the general configury on 1-2 platforms, it's not any
>> >  less tested than what you have now.
> Unfortunately, that's not true: my original patch just moved the
> existing ENABLE_EXECUTE_STACK definitions to new headers in libgcc and
> used them as is.  The revised one below consolidates them into two new
> files, only one of them has been tested so far.

Yes, I wasn't expecting to go to this length.  But thanks for doing it.

>>> >>  One other caveat: I don't know if I like grouping the configure support
>>> >>  for the enable-execute-stack.c variants together or would rather keep
>>> >>  all configuration for a single platform (or OS) together.  Probably a
>>> >>  matter of taste.
>> >
>> >  In case it wasn't clear, I was proposing the latter.
> I suppose you mean the former, i.e. grouping them together and not
> setting enable_execute_stack in the existing host cases?

I meant doing it in config.host, because I was expecting you to keep all 
implementations in separate *.c files, even for all the mprotect 
variants, but I agree your patch is better.

> Will do when I submit the final patch.  Right now, I've got a plan and a
> draft where I'm looking for comments.

Makes sense.

> * Except for Darwin, the code uses TRAMPOLINE_SIZE.  This only exists in
>    the backend headers.  While it could be duplicated somewhere in the
>    libgcc configury, I'd rather propose that gcc define
>    __TRAMPOLINE_SIZE__ (in gcc/c-family/c-cppbuiltin.c (c_cpp_builtins)
>    to avoid this.  On PowerPC Darwin, one cannot use TRAMPOLINE_SIZE
>    definition right now since the macro calls the rs6000_trampoline_size
>    function in rs6000/rs6000.c.  This would be solved nicely by
>    __TRAMPOLINE_SIZE__.

Good idea.

> * FreeBSD and Solaris use a check_enabling function to check if
>    __enable_execute_stack needs to run at all.  Initially, I had
>    enable-execute-stack-{freebsd, netbsd, sol2}.c, all including a common
>    enable-execute-stack-mprotect.c to avoid code duplication.  I now
>    consider that ugly and hard to read, and merged everything into a
>    common enable-execute-stack-mprotect.c below.  Right now, the file
>    uses __FreeBSD__, __NetBSD__, and __sun__&&  __svr4__ to select the
>    right code, but this could be autoconf'ed if desired.  There are a
>    couple of FIXME comments in the code.

Please avoid putting the ((constructor)) in common code.  You can put

static int i = 1;

in the #else case, and leave

static int i;

in common code.

> * NetBSD is extremely careful to avoid namespace pollution and uses
>    private __sysctl (declared privately) to avoid using getpagesize.
>    Either this is an issue, and we might do something similar on all
>    platforms, or it's not and we should avoid special-casing NetBSD
>    here.

Perhaps it has to do with getpagesize not being available for old 
versions of NetBSD.  No idea, but I'd leave it as is.

> * FreeBSD uses the unmodified address passed to __enable_execute_stack
>    to call mprocted, while all others round both address and size to a
>    pagesize boundary.  I cannot imagine that FreeBSD supports
>    byte-granularity mprotect, so this seems an oversight.

Agreed.

> * Windows32 calls abort when VirtualProtect fails.  With the exception
>    of Darwin and NetBSD, all others call perror, which seems to be
>    frowned upon.  We should be consistent here.

I think it should either abort() or do nothing, consistently across all 
platforms.

> * gcc/config/i386/mingw32.h has
>
> #ifdef IN_LIBGCC2
> #include<windows.h>
> #endif
>
>    I strongly suspect that this is only to declare VirtualQuery and
>    VirtualProtect and can go if ENABLE_EXECUTE_STACK is removed?

Yes, it should go.

> * Finally, __enable_execute_stack is only used on those NetBSD targets
> that actually define HAVE_ENABLE_EXECUTE_STACK, while OpenBSD uses it
> everywhere.  config.host could be simplyfied if NetBSD behaved the
> same.

I would add it to all NetBSDs, and perhaps all FreeBSDs too?  Do you 
know why SPARC is special?

Paolo
Gerald Pfeifer June 4, 2011, 11 a.m. UTC | #2
On Fri, 3 Jun 2011, Rainer Orth wrote:
> * FreeBSD uses the unmodified address passed to __enable_execute_stack
>   to call mprocted, while all others round both address and size to a
>   pagesize boundary.  I cannot imagine that FreeBSD supports
>   byte-granularity mprotect, so this seems an oversight.

The man page of mprotect on FreeBSD 9 (the next release) states the
following which seems supportive of your theory:

  NAME
     mprotect -- control the protection of pages
  :
  DESCRIPTION
     The mprotect() system call changes the specified pages to have protection
     prot.  Not all implementations will guarantee protection on a page basis;
     the granularity of protection changes may be as large as an entire
     region.  A region is the virtual address space defined by the start and
     end addresses of a struct vm_map_entry.


Gerald
Andreas Schwab June 4, 2011, 11:17 a.m. UTC | #3
Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:

> * FreeBSD uses the unmodified address passed to __enable_execute_stack
>   to call mprocted, while all others round both address and size to a
>   pagesize boundary.  I cannot imagine that FreeBSD supports
>   byte-granularity mprotect, so this seems an oversight.

Apparently freebsd's mprotect aligns the address by itself, so it will
not make any difference.  At least linux's mprotect will barf if the
address is unaligned.

Andreas.
diff mbox

Patch

diff --git a/gcc/libgcc2.c b/gcc/libgcc2.c
--- a/gcc/libgcc2.c
+++ b/gcc/libgcc2.c
@@ -1,7 +1,7 @@ 
 /* More subroutines needed by GCC output code on some machines.  */
 /* Compile this one with gcc.  */
 /* Copyright (C) 1989, 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999,
-   2000, 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2009, 2010
+   2000, 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2009, 2010, 2011
    Free Software Foundation, Inc.
 
 This file is part of GCC.
@@ -2027,19 +2027,6 @@  __clear_cache (char *beg __attribute__((
 
 #endif /* L_clear_cache */
 
-#ifdef L_enable_execute_stack
-/* Attempt to turn on execute permission for the stack.  */
-
-#ifdef ENABLE_EXECUTE_STACK
-  ENABLE_EXECUTE_STACK
-#else
-void
-__enable_execute_stack (void *addr __attribute__((__unused__)))
-{}
-#endif /* ENABLE_EXECUTE_STACK */
-
-#endif /* L_enable_execute_stack */
-
 #ifdef L_trampoline
 
 /* Jump to a trampoline, loading the static chain address.  */
diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in
--- a/libgcc/Makefile.in
+++ b/libgcc/Makefile.in
@@ -313,9 +313,11 @@  ifneq ($(GCC_EXTRA_PARTS),)
 endif
 endif
 
+LIB2ADD += enable-execute-stack.c
+
 # Library members defined in libgcc2.c.
 lib2funcs = _muldi3 _negdi2 _lshrdi3 _ashldi3 _ashrdi3 _cmpdi2 _ucmpdi2	   \
-	    _clear_cache _enable_execute_stack _trampoline __main _absvsi2 \
+	    _clear_cache _trampoline __main _absvsi2 \
 	    _absvdi2 _addvsi3 _addvdi3 _subvsi3 _subvdi3 _mulvsi3 _mulvdi3 \
 	    _negvsi2 _negvdi2 _ctors _ffssi2 _ffsdi2 _clz _clzsi2 _clzdi2  \
 	    _ctzsi2 _ctzdi2 _popcount_tab _popcountsi2 _popcountdi2	   \
diff --git a/libgcc/config.host b/libgcc/config.host
--- a/libgcc/config.host
+++ b/libgcc/config.host
@@ -44,6 +44,8 @@ 
 #			The default is ".hidden".
 #  cpu_type		The name of the cpu, if different from the first
 #			chunk of the canonical host name.
+#  enable_execute_stack The name of a source file implementing
+#			__enable_execute_stack.
 #  extra_parts		List of extra object files that should be compiled
 #			for this target machine.  This may be overridden
 #			by setting EXTRA_PARTS in a tmake_file fragment.
@@ -57,6 +59,7 @@ 
 #			"$cpu_type/t-$cpu_type".
 
 asm_hidden_op=.hidden
+enable_execute_stack=
 extra_parts=
 tmake_file=
 md_unwind_header=no-unwind.h
@@ -202,6 +205,26 @@  case ${host} in
   ;;
 esac
 
+# FIXME: Apply to *-*-netbsd*?
+case ${host} in
+*-*-darwin* | \
+  *-*-openbsd* | \
+  *-*-solaris2* | \
+  alpha*-dec-osf5.1* | \
+  alpha*-*-netbsd* | \
+  i[34567]86-*-netbsdelf* | x86_64-*-netbsd* | \
+  sparc-*-netbsdelf* | sparc64-*-netbsd* | \
+  sparc64-*-freebsd* | ultrasparc-*-freebsd*)
+  enable_execute_stack=config/enable-execute-stack-mprotect.c
+  ;;
+i[34567]86-*-mingw* | x86_64-*-mingw*)
+  enable_execute_stack=config/i386/enable-execute-stack-mingw32.c
+  ;;
+*)
+  enable_execute_stack=enable-execute-stack-empty.c;
+  ;;
+esac
+
 case ${host} in
 # Support site-specific machine types.
 *local*)
diff --git a/libgcc/config/enable-execute-stack-mprotect.c b/libgcc/config/enable-execute-stack-mprotect.c
new file mode 100644
--- /dev/null
+++ b/libgcc/config/enable-execute-stack-mprotect.c
@@ -0,0 +1,114 @@ 
+/* Implement __enable_execute_stack using mprotect(2).
+   Copyright (C) 2011 Free Software Foundation, Inc.
+
+   This file is part of GCC.
+
+   GCC 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.
+
+   GCC 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.
+
+   Under Section 7 of GPL version 3, you are granted additional
+   permissions described in the GCC Runtime Library Exception, version
+   3.1, as published by the Free Software Foundation.
+
+   You should have received a copy of the GNU General Public License and
+   a copy of the GCC Runtime Library Exception along with this program;
+   see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <sys/mman.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+#define STACK_PROT_RWX (PROT_READ | PROT_WRITE | PROT_EXEC)
+
+static int need_enable_exec_stack;
+
+static void check_enabling(void) __attribute__ ((constructor));
+extern void __enable_execute_stack (void *);
+
+#if defined __FreeBSD__
+/* FIXME: Use HAVE_SYSCTLBYNAME instead?  Need libgcc config.h for that,
+   and perhaps check for kern.stackprot.  */
+#include <sys/sysctl.h>
+
+static void
+check_enabling (void)
+{
+  int prot = 0;
+  size_t len = sizeof (prot);
+
+  sysctlbyname ("kern.stackprot", &prot, &len, NULL, 0);
+  if (prot != STACK_PROT_RWX)
+    need_enable_exec_stack = 1;
+}
+#elif defined __sun__ && defined __svr4__
+/* FIXME: Use defined _SC_STACK_PROT instead?  */
+static void
+check_enabling (void)
+{
+  int prot = (int) sysconf (_SC_STACK_PROT);
+
+  if (prot != STACK_PROT_RWX)
+    need_enable_exec_stack = 1;
+}
+#else
+static void
+check_enabling (void)
+{
+  need_enable_exec_stack = 1;
+}
+#endif
+
+#if defined __NetBSD__
+/* FIXME: Use HAVE___SYSCTL instead?  */
+#include <sys/sysctl.h>
+
+/* FIXME: Include comment wrt. namespace cleanlyness.  */
+/* FIXME: Header?  */
+extern int __sysctl (int *, unsigned int, void *, size_t *, void *, size_t);
+
+static int
+getpagesize (void)
+{
+  static int size;
+
+  if (size == 0)
+    {
+      int mib[2];
+      size_t len;
+
+      mib[0] = CTL_HW;
+      mib[1] = HW_PAGESIZE;
+      len = sizeof (size);
+      (void) __sysctl (mib, 2, &size, &len, NULL, 0);
+    }
+  return size;
+}
+#endif /* __NetBSD__ */
+
+void
+__enable_execute_stack (void *addr)
+{
+  if (!need_enable_exec_stack)
+    return;
+  else
+    {
+      long size = getpagesize ();
+      long mask = ~(size - 1);
+      char *page = (char *) (((long) addr) & mask);
+      char *end  = (char *) ((((long) (addr + __TRAMPOLINE_SIZE__)) & mask)
+			     + size);
+
+      /* FIXME: Use addr, __TRAMPOLINE_SIZE__ directly on FreeBSD!?  */
+      if (mprotect (page, end - page, STACK_PROT_RWX) < 0)
+	/* FIXME: Make conditional, use abort instead?  */
+	perror ("mprotect of trampoline code");
+    }
+}
diff --git a/libgcc/config/i386/enable-execute-stack-mingw32.c b/libgcc/config/i386/enable-execute-stack-mingw32.c
new file mode 100644
--- /dev/null
+++ b/libgcc/config/i386/enable-execute-stack-mingw32.c
@@ -0,0 +1,16 @@ 
+/* Implement __enable_execute_stack for Windows32.  */
+
+#include <windows.h>
+
+extern void __enable_execute_stack (void *);
+
+void
+__enable_execute_stack (void *addr)
+{
+  MEMORY_BASIC_INFORMATION b;
+
+  if (!VirtualQuery (addr, &b, sizeof(b)))
+    abort ();
+  VirtualProtect (b.BaseAddress, b.RegionSize, PAGE_EXECUTE_READWRITE,
+		  &b.Protect);
+}
diff --git a/libgcc/configure.ac b/libgcc/configure.ac
--- a/libgcc/configure.ac
+++ b/libgcc/configure.ac
@@ -278,6 +278,7 @@  AC_SUBST(tmake_file)
 AC_SUBST(cpu_type)
 AC_SUBST(extra_parts)
 AC_SUBST(asm_hidden_op)
+AC_CONFIG_LINKS([enable-execute-stack.c:$enable_execute_stack])
 AC_CONFIG_LINKS([md-unwind-support.h:config/$md_unwind_header])
 
 # We need multilib support.
diff --git a/libgcc/enable-execute-stack-empty.c b/libgcc/enable-execute-stack-empty.c
new file mode 100644
--- /dev/null
+++ b/libgcc/enable-execute-stack-empty.c
@@ -0,0 +1,7 @@ 
+/* Dummy implementation of __enable_execute_stack.  */
+
+/* Attempt to turn on execute permission for the stack.  */
+void
+__enable_execute_stack (void *addr __attribute__((__unused__)))
+{
+}