Re: [PATCH] add a configure option for using RELRO by default

[Romain Geissler <romain dot geissler at amadeus dot com>]

On Tue, 10 Nov 2015, Mike Frysinger wrote:

> On 10 Nov 2015 22:16, Romain Geissler wrote:
> > --- a/gold/configure.ac
> > +++ b/gold/configure.ac
> >
> > +# Decide whether you want to set "-z relro" by default
>
> should be a dnl instead of a # ?
>
> > +ac_default_relro=unset

With dnl this comment will not be in the generated configure file. With #
it will. For me it might help to have this comment when investigating
issues in configure. So I let it this way. Unless you want me to really
switch to dnl.

>
> i don't think we want the ac_ prefix since this isn't autoconf code.
>
> would be better too imo to make this the 4th arg to AC_ARG_ENABLE below.
>
> > +  *) AC_MSG_ERROR(bad value ${enableval} for default-relro option) ;;
>
> should quote the arg with []
>
> > +if test x$ac_default_relro == xyes ; then
>
> quote the LHS and change the == to =
>
> > +  AC_DEFINE(DEFAULT_RELRO, 1, [Define if you want to use read only relocations by default])
>
> quote the 1st & 2nd arg with []
>
> > +#ifdef DEFAULT_RELRO
> > +#define DEFAULT_RELRO_VALUE true
> > +#else
> > +#define DEFAULT_RELRO_VALUE false
> > +#endif
>
> should use "# define" imo
>
> > --- a/ld/configure.ac
> > +++ b/ld/configure.ac
>
> same feedback here as for gold
> -mike
>

I implemented that in v3.

gold/ChangeLog:
2015-11-10  Romain Geissler  <romain.geissler@amadeus.com>

	* configure.ac: Add --enable-default-relro switch.
	* options.cc (General_options::finalize): Disable relro if not set
	explicitly when linking incrementally.
	* options.h (General_options): Handle DEFAULT_RELRO.
	* config.in: Regenerate.
	* configure: Regenerate.
	* Makefile.in: Regenerate.

ld/ChangeLog:
2015-11-10  Romain Geissler  <romain.geissler@amadeus.com>

	* configure.ac: Add --enable-default-relro switch.
	* emultempl/elf32.em: Handle DEFAULT_RELRO.
	* testsuite/config/default.exp: Disable RELRO.
	* testsuite/ld-bootstrap/bootstrap.exp: Disable RELRO.
	* config.in: Regenerate.
	* configure: Regenerate.

ld/testsuite/ChangeLog:
2015-11-10  Romain Geissler  <romain.geissler@amadeus.com>

	* config/default.exp (ld, LD, ld_L_opt): Append -z norelro for ELF targets.
	* ld-bootstrap/bootstrap.exp (ldexe): New.



diff --git a/gold/Makefile.in b/gold/Makefile.in
index dbfde80..d04378e 100644
--- a/gold/Makefile.in
+++ b/gold/Makefile.in
@@ -70,8 +70,8 @@ subdir = .
 DIST_COMMON = NEWS README ChangeLog $(srcdir)/Makefile.in \
 	$(srcdir)/Makefile.am $(top_srcdir)/configure \
 	$(am__configure_deps) $(srcdir)/config.in \
-	$(srcdir)/../mkinstalldirs $(top_srcdir)/po/Make-in pread.c \
-	ffsll.c ftruncate.c mremap.c yyscript.h yyscript.c \
+	$(srcdir)/../mkinstalldirs $(top_srcdir)/po/Make-in ffsll.c \
+	ftruncate.c pread.c mremap.c yyscript.h yyscript.c \
 	$(srcdir)/../depcomp $(srcdir)/../ylwrap
 ACLOCAL_M4 = $(top_srcdir)/aclocal.m4
 am__aclocal_m4_deps = $(top_srcdir)/../config/depstand.m4 \
diff --git a/gold/config.in b/gold/config.in
index 88e8712..fe6190b 100644
--- a/gold/config.in
+++ b/gold/config.in
@@ -10,6 +10,9 @@
 /* Define if building universal (internal helper macro) */
 #undef AC_APPLE_UNIVERSAL_BUILD

+/* Define if you want to use read only relocations by default */
+#undef DEFAULT_RELRO
+
 /* Define to 1 if translation of program messages to the user's native
    language is requested. */
 #undef ENABLE_NLS
diff --git a/gold/configure b/gold/configure
index 987a846..4b2ffc4 100755
--- a/gold/configure
+++ b/gold/configure
@@ -791,6 +791,7 @@ enable_gold
 enable_threads
 enable_plugins
 enable_targets
+enable_default_relro
 with_lib_path
 enable_dependency_tracking
 enable_nls
@@ -1440,6 +1441,7 @@ Optional Features:
   --enable-threads        multi-threaded linking
   --enable-plugins        linker plugins
   --enable-targets        alternative target configurations
+  --enable-default-relro  mark relocations read-only by default
   --disable-dependency-tracking  speeds up one-time build
   --enable-dependency-tracking   do not reject slow dependency extractors
   --disable-nls           do not use Native Language Support
@@ -3384,6 +3386,25 @@ if test -n "$enable_targets"; then
   done
 fi

+# Decide whether you want to set "-z relro" by default
+# Check whether --enable-default-relro was given.
+if test "${enable_default_relro+set}" = set; then :
+  enableval=$enable_default_relro; case "${enableval}" in
+  yes) default_relro=yes ;;
+  no) ac_default_relro=no ;;
+  *) as_fn_error "bad value ${enableval} for default-relro option" "$LINENO" 5 ;;
+esac
+else
+  default_relro=unset
+fi
+
+
+if test "x$default_relro" = "xyes" ; then
+
+$as_echo "#define DEFAULT_RELRO 1" >>confdefs.h
+
+fi
+
 # See which specific instantiations we need.
 targetobjs=
 all_targets=
diff --git a/gold/configure.ac b/gold/configure.ac
index 89f6c53..d4a4b39 100644
--- a/gold/configure.ac
+++ b/gold/configure.ac
@@ -144,6 +144,19 @@ if test -n "$enable_targets"; then
   done
 fi

+# Decide whether you want to set "-z relro" by default
+AC_ARG_ENABLE([default-relro],
+              AS_HELP_STRING([--enable-default-relro], [mark relocations read-only by default]),
+[case "${enableval}" in
+  yes) default_relro=yes ;;
+  no) ac_default_relro=no ;;
+  *) AC_MSG_ERROR([bad value ${enableval} for default-relro option]) ;;
+esac], default_relro=unset)
+
+if test "x$default_relro" = "xyes" ; then
+  AC_DEFINE([DEFAULT_RELRO], [1], [Define if you want to use read only relocations by default])
+fi
+
 # See which specific instantiations we need.
 targetobjs=
 all_targets=
diff --git a/gold/options.cc b/gold/options.cc
index c42623f..2c1994a 100644
--- a/gold/options.cc
+++ b/gold/options.cc
@@ -1279,7 +1279,12 @@ General_options::finalize()
       if (this->has_plugins())
 	gold_fatal(_("incremental linking is not compatible with --plugin"));
       if (this->relro())
-	gold_fatal(_("incremental linking is not compatible with -z relro"));
+      {
+        if (this->user_set_relro())
+          gold_fatal(_("incremental linking is not compatible with -z relro"));
+        else
+          this->set_relro(false);
+      }
       if (this->gc_sections())
 	{
 	  gold_warning(_("ignoring --gc-sections for an incremental link"));
diff --git a/gold/options.h b/gold/options.h
index ffc44e6..5cf003a 100644
--- a/gold/options.h
+++ b/gold/options.h
@@ -1332,7 +1332,12 @@ class General_options
   DEFINE_bool(origin, options::DASH_Z, '\0', false,
 	      N_("Mark DSO to indicate that needs immediate $ORIGIN "
 		 "processing at runtime"), NULL);
-  DEFINE_bool(relro, options::DASH_Z, '\0', false,
+#ifdef DEFAULT_RELRO
+# define DEFAULT_RELRO_VALUE true
+#else
+# define DEFAULT_RELRO_VALUE false
+#endif
+  DEFINE_bool(relro, options::DASH_Z, '\0', DEFAULT_RELRO_VALUE,
 	      N_("Where possible mark variables read-only after relocation"),
 	      N_("Don't mark variables read-only after relocation"));
   DEFINE_bool(text, options::DASH_Z, '\0', false,
diff --git a/ld/config.in b/ld/config.in
index 276fb77..002002c 100644
--- a/ld/config.in
+++ b/ld/config.in
@@ -10,6 +10,9 @@
 /* Define if you want compressed debug sections by default. */
 #undef DEFAULT_FLAG_COMPRESS_DEBUG

+/* Define if you want to use read only relocations by default */
+#undef DEFAULT_RELRO
+
 /* Define to 1 if translation of program messages to the user's native
    language is requested. */
 #undef ENABLE_NLS
diff --git a/ld/configure b/ld/configure
index b41efe8..4ea45a8 100755
--- a/ld/configure
+++ b/ld/configure
@@ -789,6 +789,7 @@ with_sysroot
 enable_gold
 enable_got
 enable_compressed_debug_sections
+enable_default_relro
 enable_werror
 enable_build_warnings
 enable_nls
@@ -1447,6 +1448,7 @@ Optional Features:
                           multigot)
   --enable-compressed-debug-sections={all,ld,none}
                           compress debug sections by default]
+  --enable-default-relro  mark relocations read-only by default
   --enable-werror         treat compile warnings as errors
   --enable-build-warnings enable build-time compiler warnings
   --disable-nls           do not use Native Language Support
@@ -11716,7 +11718,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 11719 "configure"
+#line 11721 "configure"
 #include "confdefs.h"

 #if HAVE_DLFCN_H
@@ -11822,7 +11824,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 11825 "configure"
+#line 11827 "configure"
 #include "confdefs.h"

 #if HAVE_DLFCN_H
@@ -15536,6 +15538,25 @@ if test "${enable_compressed_debug_sections+set}" = set; then :
 esac
 fi

+# Decide whether you want to set "-z relro" by default
+# Check whether --enable-default-relro was given.
+if test "${enable_default_relro+set}" = set; then :
+  enableval=$enable_default_relro; case "${enableval}" in
+  yes) default_relro=yes ;;
+  no) ac_default_relro=no ;;
+  *) as_fn_error "bad value ${enableval} for default-relro option" "$LINENO" 5 ;;
+esac
+else
+  default_relro=unset
+fi
+
+
+if test "x$default_relro" = "xyes" ; then
+
+$as_echo "#define DEFAULT_RELRO 1" >>confdefs.h
+
+fi
+

 # Set the 'development' global.
 . $srcdir/../bfd/development.sh
diff --git a/ld/configure.ac b/ld/configure.ac
index 188172d..dbd6186 100644
--- a/ld/configure.ac
+++ b/ld/configure.ac
@@ -155,6 +155,19 @@ AC_ARG_ENABLE(compressed_debug_sections,
   ,no, | ,none,)  ac_default_compressed_debug_sections=no ;;
 esac])dnl

+# Decide whether you want to set "-z relro" by default
+AC_ARG_ENABLE([default-relro],
+              AS_HELP_STRING([--enable-default-relro], [mark relocations read-only by default]),
+[case "${enableval}" in
+  yes) default_relro=yes ;;
+  no) ac_default_relro=no ;;
+  *) AC_MSG_ERROR([bad value ${enableval} for default-relro option]) ;;
+esac], default_relro=unset)
+
+if test "x$default_relro" = "xyes" ; then
+  AC_DEFINE([DEFAULT_RELRO], [1], [Define if you want to use read only relocations by default])
+fi
+
 AM_BINUTILS_WARNINGS

 AM_LC_MESSAGES
diff --git a/ld/emultempl/elf32.em b/ld/emultempl/elf32.em
index 0405d4f..d991c16 100644
--- a/ld/emultempl/elf32.em
+++ b/ld/emultempl/elf32.em
@@ -104,6 +104,9 @@ gld${EMULATION_NAME}_before_parse (void)
   config.has_shared = `if test -n "$GENERATE_SHLIB_SCRIPT" ; then echo TRUE ; else echo FALSE ; fi`;
   config.separate_code = `if test "x${SEPARATE_CODE}" = xyes ; then echo TRUE ; else echo FALSE ; fi`;
   `if test -n "$CALL_NOP_BYTE" ; then echo link_info.call_nop_byte = $CALL_NOP_BYTE; fi`;
+#ifdef DEFAULT_RELRO
+  link_info.relro = TRUE;
+#endif
 }

 EOF
diff --git a/ld/testsuite/config/default.exp b/ld/testsuite/config/default.exp
index 310a3b2..d74cdd3 100644
--- a/ld/testsuite/config/default.exp
+++ b/ld/testsuite/config/default.exp
@@ -21,8 +21,16 @@
 # Written by Jeffrey Wheat (cassidy@cygnus.com)
 #

+# load the utility procedures
+load_lib ld-lib.exp
+
 if ![info exists ld] then {
     set ld [findfile $base_dir/ld-new $base_dir/ld-new [transform ld]]
+
+    # Make sure tests pass even if configured with --enable-default-relro
+    if {[is_elf_format]} then {
+        append ld " -z norelro"
+    }
 }

 if ![info exists as] then {
@@ -76,6 +84,11 @@ if {[file exists tmpdir/libpath.exp]} {
     }
 }

+# Make sure tests pass even if configured with --enable-default-relro
+if {[is_elf_format]} then {
+    append ld_L_opt " -z norelro"
+}
+
 # The "make check" target in the Makefile passes in
 # "CC=$(CC_FOR_TARGET)".  But, if the user invokes runtest directly
 # (as when testing an installed linker), these flags may not be set.
@@ -108,9 +121,6 @@ if { [istarget rx-*-*] } {
     set ASFLAGS "-muse-conventional-section-names"
 }

-# load the utility procedures
-load_lib ld-lib.exp
-
 proc get_link_files {varname} {
     global $varname
     global target_triplet
@@ -277,6 +287,11 @@ if ![info exists READELFFLAGS] then {

 if ![info exists LD] then {
     set LD [findfile $base_dir/ld-new ./ld-new [transform ld]]
+
+    # Make sure tests pass even if configured with --enable-default-relro
+    if {[is_elf_format]} then {
+        append LD " -z norelro"
+    }
 }

 if ![info exists LDFLAGS] then {
diff --git a/ld/testsuite/ld-bootstrap/bootstrap.exp b/ld/testsuite/ld-bootstrap/bootstrap.exp
index 3b6eb84..749bd9a 100644
--- a/ld/testsuite/ld-bootstrap/bootstrap.exp
+++ b/ld/testsuite/ld-bootstrap/bootstrap.exp
@@ -78,7 +78,13 @@ foreach flags $test_flags {

     # This test can only be run if we have the ld build directory,
     # since we need the object files.
-    if {$ld != "$objdir/ld-new"} {
+    set ldexe $ld
+    set ldparm [string first " " $ld]
+    if { $ldparm > 0 } then {
+        set ldparm [expr $ldparm - 1]
+        set ldexe [string range $ld 0 $ldparm]
+    }
+    if {$ldexe != "$objdir/ld-new"} {
 	untested $testname
 	continue
     }