diff mbox

fix PR sanitizer/55617 via qsort

Message ID 20130204172259.GA23606@bromo.med.uc.edu
State New
Headers show

Commit Message

Jack Howarth Feb. 4, 2013, 5:22 p.m. UTC
Currently darwin is unable to utilize libasan with constructors due to the lack of
constructor priority support on that target. The asan_finish_file routine inserts an
essential __asan_init into the array of constructors (via the __mod_init_func section).
However the insertion occurs at the end, and due to the lack of priority support for
constructors, these are executed from the front of the array of constructors on program
startup. This causes code any instrumented code that executes before the __asan_init
call to crash. 
   The attached patch uses a va_gc vector of constructor symbol/priority records to
queue this data inside machopic_asm_out_constructor. When the ctors vector is not empty,
the finalize_ctors routine is called in darwin_file_end to sort the queue by priority,
using a qsort stabilized on original position for identical priority, prior to emitting
the constructors. The patch also adds a g++.dg/asan/pr55617.C test case which is
targeted to i?86-*-darwin* and x86_64-*-darwin*.
    The patch reduces the failures observed when running....

make -k check-g++ RUNTESTFLAGS="--target_board=unix'{-fsanitize=address}'"

from 323 to only 85 (which is similar to what linux shows). The cov.C testcase also
fails on gcc trunk with -fsanitize=address when recrafted into a dynamic shared library
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55617#c28. This patch eliminates those
crashes. This problem doesn't extend to when the shared library or module is dlopen'd
(which works in stock gcc trunk and with this patch as well).
    The patch has been bootstrap and regression tested on x86_64-apple-darwin12.
Okay for gcc trunk?
         Jack
ps The issue of inter module priority support remains unresolved (as it is in clang/llvm).
The only solution for both compilers is to reorder the linkage of the modules to insure that the
module with the asan constructor appears first.
/gcc

2013-02-04  Alexander Potapenko <glider@google.com>
            Jack Howarth  <howarth@bromo.med.uc.edu>
	    Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/55617
	* config/darwin.c (sort_ctor_records): Stabilized qsort
	on constructor priority by using original position.
	(finalize_ctors): New routine to sort constructors by
	priority before use in assemble_integer.
	(machopic_asm_out_constructor): Use finalize_ctors if needed.

/gcc/testsuite

2013-02-04  Alexander Potapenko <glider@google.com>
            Jack Howarth  <howarth@bromo.med.uc.edu>
	    Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/55617
	* g++.dg/asan/pr55617.C: New test.

Comments

Mike Stump Feb. 4, 2013, 7:23 p.m. UTC | #1
On Feb 4, 2013, at 9:22 AM, Jack Howarth <howarth@bromo.med.uc.edu> wrote:
>   Currently darwin is unable to utilize libasan with constructors due to the lack of
> constructor priority support on that target.

> Okay for gcc trunk?

Since asan doesn't need cross translation unit priorities, the patch is sufficient to fix all of the semantics needed for asan.

I still have a preference, though small, for stable_sort instead of qsort, absent performance data saying qsort is better.

Ok.

> ps The issue of inter module priority support remains unresolved (as it is in clang/llvm).
> The only solution for both compilers is to reorder the linkage of the modules to insure that the module with the asan constructor appears first.

Since asan doesn't care who goes first, we don't need priorities across translation units for it.
Mike Stump Feb. 4, 2013, 7:27 p.m. UTC | #2
On Feb 4, 2013, at 11:23 AM, Mike Stump <mrs@mrs.kithrup.com> wrote:
> On Feb 4, 2013, at 9:22 AM, Jack Howarth <howarth@bromo.med.uc.edu> wrote:
>>  Currently darwin is unable to utilize libasan with constructors due to the lack of
>> constructor priority support on that target.
> 
>> Okay for gcc trunk?
> 
> Since asan doesn't need cross translation unit priorities, the patch is sufficient to fix all of the semantics needed for asan.
> 
> I still have a preference, though small, for stable_sort instead of qsort, absent performance data saying qsort is better.
> 
> Ok.

Oh, if you could, could you qsort of the dtors as well.  The code exactly mirrors the ctors code.
Mike Stump Feb. 4, 2013, 8:12 p.m. UTC | #3
On Feb 4, 2013, at 11:23 AM, Mike Stump <mrs@mrs.kithrup.com> wrote:
> On Feb 4, 2013, at 9:22 AM, Jack Howarth <howarth@bromo.med.uc.edu> wrote:
>>  Currently darwin is unable to utilize libasan with constructors due to the lack of
>> constructor priority support on that target.
> 
>> Okay for gcc trunk?
> 
> Since asan doesn't need cross translation unit priorities, the patch is sufficient to fix all of the semantics needed for asan.

> Ok.

Committed revision 195735.

Note, this doesn't have the test case in it.  Please repost just the test case, thanks.
diff mbox

Patch

Index: gcc/config/darwin.c
===================================================================
--- gcc/config/darwin.c	(revision 195685)
+++ gcc/config/darwin.c	(working copy)
@@ -83,6 +83,14 @@  along with GCC; see the file COPYING3.  
    kernel) the stubs might still be required, and this will be set true.  */
 int darwin_emit_branch_islands = false;
 
+typedef struct GTY(()) ctor_record {
+  rtx symbol;
+  int priority;		/* constructor priority */
+  int position;		/* original position */
+} ctor_record;
+
+static GTY(()) vec<ctor_record, va_gc> *ctors = NULL;
+
 /* A flag to determine whether we are running c++ or obj-c++.  This has to be
    settable from non-c-family contexts too (i.e. we can't use the c_dialect_
    functions).  */
@@ -1708,15 +1716,48 @@  machopic_select_rtx_section (enum machin
 void
 machopic_asm_out_constructor (rtx symbol, int priority ATTRIBUTE_UNUSED)
 {
+  ctor_record new_elt = {symbol, priority, vec_safe_length (ctors)};
+
+  vec_safe_push (ctors, new_elt);
+
+  if (! MACHOPIC_INDIRECT)
+    fprintf (asm_out_file, ".reference .constructors_used\n");
+}
+
+static int
+sort_ctor_records (const void * a, const void * b)
+{
+  const ctor_record *ca = (const ctor_record *)a;
+  const ctor_record *cb = (const ctor_record *)b;
+  if (ca->priority > cb->priority)
+    return 1;
+  if (ca->priority < cb->priority)
+    return -1;
+  if (ca->position > cb->position)
+    return 1;
+  if (ca->position < cb->position)
+    return -1;
+  return 0;
+}
+
+static void 
+finalize_ctors()
+{
+  unsigned int i;
+  ctor_record *elt;
+ 
   if (MACHOPIC_INDIRECT)
     switch_to_section (darwin_sections[mod_init_section]);
   else
     switch_to_section (darwin_sections[constructor_section]);
-  assemble_align (POINTER_SIZE);
-  assemble_integer (symbol, POINTER_SIZE / BITS_PER_UNIT, POINTER_SIZE, 1);
 
-  if (! MACHOPIC_INDIRECT)
-    fprintf (asm_out_file, ".reference .constructors_used\n");
+  if (vec_safe_length (ctors) > 1)
+    ctors->qsort (sort_ctor_records);
+  FOR_EACH_VEC_SAFE_ELT (ctors, i, elt)
+    {
+      assemble_align (POINTER_SIZE);
+      assemble_integer (elt->symbol, POINTER_SIZE / BITS_PER_UNIT, POINTER_SIZE, 1);
+    }
 }
 
 void
@@ -2762,6 +2803,8 @@  darwin_file_start (void)
 void
 darwin_file_end (void)
 {
+  if (!vec_safe_is_empty (ctors))
+    finalize_ctors();
   machopic_finish (asm_out_file);
   if (strcmp (lang_hooks.name, "GNU C++") == 0)
     {