Patchwork Put -lasan always very early on the ld command line (PR sanitizer/55374)

login
register
mail settings
Submitter Jakub Jelinek
Date Jan. 22, 2013, 4:58 p.m.
Message ID <20130122165813.GY7269@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/214603/
State New
Headers show

Comments

Jakub Jelinek - Jan. 22, 2013, 4:58 p.m.
Hi!

libasan relies on being linked before -lpthread, -lstdc++ and -lc, but so
far nothing was ensuring that.  Not only e.g. -lstdc++ could come before
-lasan when -lstdc++ was added by g++ language specific driver handling,
but also if the user specifies -lstdc++, -lpthread or -lc somewhere on
the gcc/g++ command line together with -fsanitize=address, those libraries
will likely be linked before -lasan.

This patch attempts to fix that, by (for Linux) linking -lasan as early as
possible, in particularly after -L.../ options, but before the user command
line options (input files, -Wl,* options, etc.) are copied over to the
command line (%o spec file hunk).  For -static-libasan, the patch changes
it to link -lasan only when linking executables (and PIEs), but when
linking shared libraries, we don't want to duplicate the libasan stuff in
each shared library.  And, when libasan.a is linked into executables/PIEs,
it is linked with -Bstatic --whole-archive -lasan --no-whole-archive -Bdynamic 
to avoid issues with portions of libasan.a being linked at various spots
on the command line, when it comes first, nothing would actually link it in,
and when we'd prepend it before every -lpthread, -lstdc++ and -lc on the
command line, it would be very hard to get it right (e.g. due to -Wl,-B*
user options).

This reveals that two tests don't pass with -lasan being linked before
-lstdc++, one fixed by backporting fix from upstream, the other by adjusting
the output regexp.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2013-01-22  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/55374
	* gcc.c (LIBASAN_SPEC): Define just to ADD_STATIC_LIBASAN_LIBS if
	LIBASAN_EARLY_SPEC is defined.
	(LIBASAN_EARLY_SPEC): Define to empty string if not already defined.
	(LINK_COMMAND_SPEC): Add LIBASAN_EARLY_SPEC for -fsanitize=address,
	before %o.
	* config/gnu-user.h (LIBASAN_EARLY_SPEC): Define.

	* g++.dg/asan/large-func-test-1.C: Allow both _Zna[jm] in addition
	to _Znw[jm] in the backtrace.  Allow _Zna[jm] to be the first frame
	printed in backtrace.
	* g++.dg/asan/deep-stack-uaf-1.C: Use malloc instead of operator new
	to avoid errors about mismatched allocation vs. deallocation.


	Jakub
Jason Merrill - Jan. 30, 2013, 2:49 p.m.
Looks good to me.  OK if nobody else responds today.

Jason
Dodji Seketeli - Jan. 30, 2013, 3:04 p.m.
Jakub Jelinek <jakub@redhat.com> writes:


> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2013-01-22  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR sanitizer/55374
> 	* gcc.c (LIBASAN_SPEC): Define just to ADD_STATIC_LIBASAN_LIBS if
> 	LIBASAN_EARLY_SPEC is defined.
> 	(LIBASAN_EARLY_SPEC): Define to empty string if not already defined.
> 	(LINK_COMMAND_SPEC): Add LIBASAN_EARLY_SPEC for -fsanitize=address,
> 	before %o.
> 	* config/gnu-user.h (LIBASAN_EARLY_SPEC): Define.
>
> 	* g++.dg/asan/large-func-test-1.C: Allow both _Zna[jm] in addition
> 	to _Znw[jm] in the backtrace.  Allow _Zna[jm] to be the first frame
> 	printed in backtrace.
> 	* g++.dg/asan/deep-stack-uaf-1.C: Use malloc instead of operator new
> 	to avoid errors about mismatched allocation vs. deallocation.

This looks okay for me.  Sorry for the delay.

As an aside, I am curious why about ...

> --- gcc/testsuite/g++.dg/asan/deep-stack-uaf-1.C.jj	2012-12-14 16:24:38.000000000 +0100
> +++ gcc/testsuite/g++.dg/asan/deep-stack-uaf-1.C	2013-01-22 16:43:03.337091101 +0100
> @@ -27,7 +27,7 @@ struct DeepFree<0> {
>  };
>  
>  int main() {
> -  char *x = new char[10];
> +  char *x = (char*)malloc(10);
>    // deep_free(x);
>    DeepFree<200>::free(x);
>    return x[5];

... why/how this is falling when -lasan comes before -lstdc++?
Jakub Jelinek - Jan. 30, 2013, 3:12 p.m.
On Wed, Jan 30, 2013 at 04:04:55PM +0100, Dodji Seketeli wrote:
> As an aside, I am curious why about ...
> 
> > --- gcc/testsuite/g++.dg/asan/deep-stack-uaf-1.C.jj	2012-12-14 16:24:38.000000000 +0100
> > +++ gcc/testsuite/g++.dg/asan/deep-stack-uaf-1.C	2013-01-22 16:43:03.337091101 +0100
> > @@ -27,7 +27,7 @@ struct DeepFree<0> {
> >  };
> >  
> >  int main() {
> > -  char *x = new char[10];
> > +  char *x = (char*)malloc(10);
> >    // deep_free(x);
> >    DeepFree<200>::free(x);
> >    return x[5];
> 
> ... why/how this is falling when -lasan comes before -lstdc++?

libasan has added recently support for operator new and operator new[]
checking, which checks that there is not a mismatch between the way
how object is allocated (malloc/calloc/realloc vs. operator new vs. operator
new[]) and how it is deallocated (free vs. delete vs. delete[]).  The
deep-stack-uaf-1.C testcase has been violating this (new char[10]
allocation, free (x) deallocation) and has been changed upstream
when those changes landed into libasan.  When -lstdc++ came first,
the -lasan operator new interceptor wasn't called (because libstdc++'s
operator new was earlier in search scope) and thus it appeared as calling
malloc and doing free to -lasan.

	Jakub

Patch

--- gcc/gcc.c.jj	2013-01-13 13:23:38.000000000 +0100
+++ gcc/gcc.c	2013-01-22 14:21:06.335193713 +0100
@@ -548,7 +548,9 @@  proper position among the other output f
 #else
 #define ADD_STATIC_LIBASAN_LIBS
 #endif
-#ifdef HAVE_LD_STATIC_DYNAMIC
+#ifdef LIBASAN_EARLY_SPEC
+#define LIBASAN_SPEC ADD_STATIC_LIBASAN_LIBS
+#elif defined(HAVE_LD_STATIC_DYNAMIC)
 #define LIBASAN_SPEC "%{static-libasan:" LD_STATIC_OPTION \
 		     "} -lasan %{static-libasan:" LD_DYNAMIC_OPTION "}" \
 		     ADD_STATIC_LIBASAN_LIBS
@@ -557,6 +559,10 @@  proper position among the other output f
 #endif
 #endif
 
+#ifndef LIBASAN_EARLY_SPEC
+#define LIBASAN_EARLY_SPEC ""
+#endif
+
 #ifndef LIBTSAN_SPEC
 #ifdef HAVE_LD_STATIC_DYNAMIC
 #define LIBTSAN_SPEC "%{static-libtsan:" LD_STATIC_OPTION \
@@ -705,7 +711,8 @@  proper position among the other output f
    "%{fuse-ld=*:-fuse-ld=%*}\
     %X %{o*} %{e*} %{N} %{n} %{r}\
     %{s} %{t} %{u*} %{z} %{Z} %{!nostdlib:%{!nostartfiles:%S}}\
-    %{static:} %{L*} %(mfwrap) %(link_libgcc) %o\
+    %{static:} %{L*} %(mfwrap) %(link_libgcc) \
+    %{fsanitize=address:" LIBASAN_EARLY_SPEC "} %o\
     %{fopenmp|ftree-parallelize-loops=*:%:include(libgomp.spec)%(link_gomp)}\
     %{fgnu-tm:%:include(libitm.spec)%(link_itm)}\
     %(mflib) " STACK_SPLIT_SPEC "\
--- gcc/config/gnu-user.h.jj	2013-01-11 09:03:09.000000000 +0100
+++ gcc/config/gnu-user.h	2013-01-22 14:22:43.333639647 +0100
@@ -98,6 +98,15 @@  see the files COPYING3 and COPYING.RUNTI
 #define TARGET_C99_FUNCTIONS 1
 #define TARGET_HAS_SINCOS 1
 
+/* Link -lasan early on the command line.  For -static-libasan, don't link
+   it for -shared link, the executable should be compiled with -static-libasan
+   in that case, and for executable link link with --{,no-}whole-archive around
+   it to force everything into the executable.  */
+#undef LIBASAN_EARLY_SPEC
+#define LIBASAN_EARLY_SPEC "%{static-libasan:%{!shared:" \
+  LD_STATIC_OPTION " --whole-archive -lasan --no-whole-archive " \
+  LD_DYNAMIC_OPTION "}}%{!static-libasan:-lasan}"
+
 /* Additional libraries needed by -static-libasan.  */
 #undef STATIC_LIBASAN_LIBS
 #define STATIC_LIBASAN_LIBS "-ldl -lpthread"
--- gcc/testsuite/g++.dg/asan/large-func-test-1.C.jj	2012-12-14 16:24:38.000000000 +0100
+++ gcc/testsuite/g++.dg/asan/large-func-test-1.C	2013-01-22 16:49:31.158090177 +0100
@@ -41,5 +41,5 @@  int main() {
 // { dg-output "    #0 0x\[0-9a-f\]+ (in \[^\n\r]*LargeFunction\[^\n\r]*(large-func-test-1.C:18|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" }
 // { dg-output "0x\[0-9a-f\]+ is located 44 bytes to the right of 400-byte region.*(\n|\r\n|\r)" }
 // { dg-output "allocated by thread T0 here:\[^\n\r]*(\n|\r\n|\r)" }
-// { dg-output "    #0 0x\[0-9a-f\]+ (in _*(interceptor_|)malloc|\[(\])\[^\n\r]*(\n|\r\n|\r)" }
-// { dg-output "    #1 0x\[0-9a-f\]+ (in (operator new|_*_Znw\[mj\])|\[(\])\[^\n\r]*(\n|\r\n|\r)" }
+// { dg-output "    #0( 0x\[0-9a-f\]+ (in _*(interceptor_|)malloc|\[(\])\[^\n\r]*(\n|\r\n|\r)" }
+// { dg-output "    #1|) 0x\[0-9a-f\]+ (in (operator new|_*_Zn\[aw\]\[mj\])|\[(\])\[^\n\r]*(\n|\r\n|\r)" }
--- gcc/testsuite/g++.dg/asan/deep-stack-uaf-1.C.jj	2012-12-14 16:24:38.000000000 +0100
+++ gcc/testsuite/g++.dg/asan/deep-stack-uaf-1.C	2013-01-22 16:43:03.337091101 +0100
@@ -27,7 +27,7 @@  struct DeepFree<0> {
 };
 
 int main() {
-  char *x = new char[10];
+  char *x = (char*)malloc(10);
   // deep_free(x);
   DeepFree<200>::free(x);
   return x[5];