diff mbox

[v3] Support ASan ODR indicators at compiler side.

Message ID 583FFAA7.4040200@samsung.com
State New
Headers show

Commit Message

Maxim Ostapenko Dec. 1, 2016, 10:25 a.m. UTC
Hi,

this is the third attempt to support ASan odr indicators in GCC. I've 
fixed the issue with odr indicator name (now we don't use 
ASM_GENERATE_INTERNAL_LABEL and use XALLOCAVEC for avoid overflow).
Also several style issues from previous review were covered.
How does it look now?

Tested and ASan bootstrapped on x86_64-unknown-linux-gnu.

-Maxim

Comments

Jakub Jelinek Dec. 1, 2016, 10:42 a.m. UTC | #1
On Thu, Dec 01, 2016 at 01:25:43PM +0300, Maxim Ostapenko wrote:
> +  int len = strlen (IDENTIFIER_POINTER (decl_name))
> +	    + sizeof ("__odr_asan_") + 1;

Please use size_t len instead of int len.  Why the + 1?  sizeof ("__odr_asan_")
should be already strlen ("__odr_asan_") + 1.

> +  name = XALLOCAVEC (char, len);
> +  name[len] = '\0';

This is buffer overflow.  Why do you need it at all?

> +  snprintf (name, len, "__odr_asan_%s", IDENTIFIER_POINTER (decl_name));

This should zero terminate the string.

Also, shouldn't this be followed by:
#ifndef NO_DOT_IN_LABEL
  name[sizeof ("__odr_asan") - 1] = '.';
#elif !defined(NO_DOLLAR_IN_LABEL)
  name[sizeof ("__odr_asan") - 1] = '$';
#endif

to make it not possible to clash with user symbols __odr_asan_foobar etc.
if possible (on targets which don't allow dots nor dollars in labels that is
not really possible, but at least elsewhere).

That said, if the __odr_asan* symbols are exported, it is part of ABI, so
what exactly does LLVM use in those cases?

> +/* { dg-final { scan-assembler-not ".*odr_asan_a.*" } } */
> +/* { dg-final { scan-assembler-not ".*odr_asan_b.*" } } */
> +/* { dg-final { scan-assembler ".*odr_asan_c.*" } } */

The .* on either side makes no sense, please remove those.
And, if the dot or dollar is replacing _, you need to use "odr_asan.a"
etc. in the regexps.

Otherwise LGTM.

	Jakub
diff mbox

Patch

commit 35cb5eb935bb1e29357e93f8e9a4e87fb4e9f511
Author: Maxim Ostapenko <m.ostapenko@samsung.com>
Date:   Fri Oct 28 10:22:03 2016 +0300

    Add support for ASan odr_indicator.
    
    config/
    
    	* bootstrap-asan.mk: Replace LSAN_OPTIONS=detect_leaks=0 with
    	ASAN_OPTIONS=detect_leaks=0:use_odr_indicator=1.
    
    gcc/
    
    	* asan.c (asan_global_struct): Refactor.
    	(create_odr_indicator): New function.
    	(asan_needs_odr_indicator_p): Likewise.
    	(is_odr_indicator): Likewise.
    	(asan_add_global): Introduce odr_indicator_ptr. Pass it into global's
    	constructor.
    	(asan_protect_global): Do not protect odr indicators.
    
    gcc/testsuite/
    
    	* c-c++-common/asan/no-redundant-odr-indicators-1.c: New test.

diff --git a/config/ChangeLog b/config/ChangeLog
index ed59787..a5d5ff5 100644
--- a/config/ChangeLog
+++ b/config/ChangeLog
@@ -1,3 +1,8 @@ 
+2016-12-01  Maxim Ostapenko  <m.ostapenko@samsung.com>
+
+	* bootstrap-asan.mk: Replace LSAN_OPTIONS=detect_leaks=0 with
+	ASAN_OPTIONS=detect_leaks=0:use_odr_indicator=1.
+
 2016-11-30  Matthias Klose  <doko@ubuntu.com>
 
 	* pkg.m4: New file.
diff --git a/config/bootstrap-asan.mk b/config/bootstrap-asan.mk
index 70baaf9..e73d4c2 100644
--- a/config/bootstrap-asan.mk
+++ b/config/bootstrap-asan.mk
@@ -1,7 +1,7 @@ 
 # This option enables -fsanitize=address for stage2 and stage3.
 
 # Suppress LeakSanitizer in bootstrap.
-export LSAN_OPTIONS="detect_leaks=0"
+export ASAN_OPTIONS=detect_leaks=0:use_odr_indicator=1
 
 STAGE2_CFLAGS += -fsanitize=address
 STAGE3_CFLAGS += -fsanitize=address
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index b3cc6305..f19ac9d 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,13 @@ 
+2016-12-01  Maxim Ostapenko  <m.ostapenko@samsung.com>
+
+	* asan.c (asan_global_struct): Refactor.
+	(create_odr_indicator): New function.
+	(asan_needs_odr_indicator_p): Likewise.
+	(is_odr_indicator): Likewise.
+	(asan_add_global): Introduce odr_indicator_ptr. Pass it into global's
+	constructor.
+	(asan_protect_global): Do not protect odr indicators.
+
 2016-12-01  Jakub Jelinek  <jakub@redhat.com>
 
 	PR target/78614
diff --git a/gcc/asan.c b/gcc/asan.c
index cb5d615..9db7481 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1388,6 +1388,16 @@  asan_needs_local_alias (tree decl)
   return DECL_WEAK (decl) || !targetm.binds_local_p (decl);
 }
 
+/* Return true if DECL, a global var, is an artificial ODR indicator symbol
+   therefore doesn't need protection.  */
+
+static bool
+is_odr_indicator (tree decl)
+{
+  return (DECL_ARTIFICIAL (decl)
+	  && lookup_attribute ("asan odr indicator", DECL_ATTRIBUTES (decl)));
+}
+
 /* Return true if DECL is a VAR_DECL that should be protected
    by Address Sanitizer, by appending a red zone with protected
    shadow memory after it and aligning it to at least
@@ -1436,7 +1446,8 @@  asan_protect_global (tree decl)
       || ASAN_RED_ZONE_SIZE * BITS_PER_UNIT > MAX_OFILE_ALIGNMENT
       || !valid_constant_size_p (DECL_SIZE_UNIT (decl))
       || DECL_ALIGN_UNIT (decl) > 2 * ASAN_RED_ZONE_SIZE
-      || TREE_TYPE (decl) == ubsan_get_source_location_type ())
+      || TREE_TYPE (decl) == ubsan_get_source_location_type ()
+      || is_odr_indicator (decl))
     return false;
 
   rtl = DECL_RTL (decl);
@@ -2266,14 +2277,15 @@  asan_dynamic_init_call (bool after_p)
 static tree
 asan_global_struct (void)
 {
-  static const char *field_names[8]
+  static const char *field_names[]
     = { "__beg", "__size", "__size_with_redzone",
-	"__name", "__module_name", "__has_dynamic_init", "__location", "__odr_indicator"};
-  tree fields[8], ret;
-  int i;
+	"__name", "__module_name", "__has_dynamic_init", "__location",
+	"__odr_indicator" };
+  tree fields[ARRAY_SIZE (field_names)], ret;
+  unsigned i;
 
   ret = make_node (RECORD_TYPE);
-  for (i = 0; i < 8; i++)
+  for (i = 0; i < ARRAY_SIZE (field_names); i++)
     {
       fields[i]
 	= build_decl (UNKNOWN_LOCATION, FIELD_DECL,
@@ -2295,6 +2307,60 @@  asan_global_struct (void)
   return ret;
 }
 
+/* Create and return odr indicator symbol for DECL.
+   TYPE is __asan_global struct type as returned by asan_global_struct.  */
+
+static tree
+create_odr_indicator (tree decl, tree type)
+{
+  char *name;
+  tree uptr = TREE_TYPE (DECL_CHAIN (TYPE_FIELDS (type)));
+  tree decl_name
+    = (HAS_DECL_ASSEMBLER_NAME_P (decl) ? DECL_ASSEMBLER_NAME (decl)
+					: DECL_NAME (decl));
+  /* DECL_NAME theoretically might be NULL.  Bail out with 0 in this case.  */
+  if (decl_name == NULL_TREE)
+    return build_int_cst (uptr, 0);
+  int len = strlen (IDENTIFIER_POINTER (decl_name))
+	    + sizeof ("__odr_asan_") + 1;
+  name = XALLOCAVEC (char, len);
+  name[len] = '\0';
+  snprintf (name, len, "__odr_asan_%s", IDENTIFIER_POINTER (decl_name));
+  tree var = build_decl (UNKNOWN_LOCATION, VAR_DECL, get_identifier (name),
+			 char_type_node);
+  TREE_ADDRESSABLE (var) = 1;
+  TREE_READONLY (var) = 0;
+  TREE_THIS_VOLATILE (var) = 1;
+  DECL_GIMPLE_REG_P (var) = 0;
+  DECL_ARTIFICIAL (var) = 1;
+  DECL_IGNORED_P (var) = 1;
+  TREE_STATIC (var) = 1;
+  TREE_PUBLIC (var) = 1;
+  DECL_VISIBILITY (var) = DECL_VISIBILITY (decl);
+  DECL_VISIBILITY_SPECIFIED (var) = DECL_VISIBILITY_SPECIFIED (decl);
+
+  TREE_USED (var) = 1;
+  tree ctor = build_constructor_va (TREE_TYPE (var), 1, NULL_TREE,
+				    build_int_cst (unsigned_type_node, 0));
+  TREE_CONSTANT (ctor) = 1;
+  TREE_STATIC (ctor) = 1;
+  DECL_INITIAL (var) = ctor;
+  DECL_ATTRIBUTES (var) = tree_cons (get_identifier ("asan odr indicator"),
+				     NULL, DECL_ATTRIBUTES (var));
+  make_decl_rtl (var);
+  varpool_node::finalize_decl (var);
+  return fold_convert (uptr, build_fold_addr_expr (var));
+}
+
+/* Return true if DECL, a global var, might be overridden and needs
+   an additional odr indicator symbol.  */
+
+static bool
+asan_needs_odr_indicator_p (tree decl)
+{
+  return !DECL_ARTIFICIAL (decl) && !DECL_WEAK (decl) && TREE_PUBLIC (decl);
+}
+
 /* Append description of a single global DECL into vector V.
    TYPE is __asan_global struct type as returned by asan_global_struct.  */
 
@@ -2335,6 +2401,9 @@  asan_add_global (tree decl, tree type, vec<constructor_elt, va_gc> *v)
       assemble_alias (refdecl, DECL_ASSEMBLER_NAME (decl));
     }
 
+  tree odr_indicator_ptr
+    = (asan_needs_odr_indicator_p (decl) ? create_odr_indicator (decl, type)
+					 : build_int_cst (uptr, 0));
   CONSTRUCTOR_APPEND_ELT (vinner, NULL_TREE,
 			  fold_convert (const_ptr_type_node,
 					build_fold_addr_expr (refdecl)));
@@ -2382,8 +2451,7 @@  asan_add_global (tree decl, tree type, vec<constructor_elt, va_gc> *v)
   else
     locptr = build_int_cst (uptr, 0);
   CONSTRUCTOR_APPEND_ELT (vinner, NULL_TREE, locptr);
-  /* TODO: support ODR indicators.  */
-  CONSTRUCTOR_APPEND_ELT (vinner, NULL_TREE, build_int_cst (uptr, 0));
+  CONSTRUCTOR_APPEND_ELT (vinner, NULL_TREE, odr_indicator_ptr);
   init = build_constructor (type, vinner);
   CONSTRUCTOR_APPEND_ELT (v, NULL_TREE, init);
 }
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 964efe9..f5adade 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -57,6 +57,8 @@  static tree handle_no_address_safety_analysis_attribute (tree *, tree, tree,
 							 int, bool *);
 static tree handle_no_sanitize_undefined_attribute (tree *, tree, tree, int,
 						    bool *);
+static tree handle_asan_odr_indicator_attribute (tree *, tree, tree, int,
+						 bool *);
 static tree handle_stack_protect_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noinline_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noclone_attribute (tree *, tree, tree, int, bool *);
@@ -292,6 +294,9 @@  const struct attribute_spec c_common_attribute_table[] =
   { "no_sanitize_undefined",  0, 0, true, false, false,
 			      handle_no_sanitize_undefined_attribute,
 			      false },
+  { "asan odr indicator",     0, 0, true, false, false,
+			      handle_asan_odr_indicator_attribute,
+			      false },
   { "warning",		      1, 1, true,  false, false,
 			      handle_error_attribute, false },
   { "error",		      1, 1, true,  false, false,
@@ -591,6 +596,15 @@  handle_no_sanitize_undefined_attribute (tree *node, tree name, tree, int,
   return NULL_TREE;
 }
 
+/* Handle an "asan odr indicator" attribute; arguments as in
+   struct attribute_spec.handler.  */
+
+static tree
+handle_asan_odr_indicator_attribute (tree *, tree, tree, int, bool *)
+{
+  return NULL_TREE;
+}
+
 /* Handle a "stack_protect" attribute; arguments as in
    struct attribute_spec.handler.  */
 
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index bf4bd8a..e570594 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,7 @@ 
+2016-12-01  Maxim Ostapenko  <m.ostapenko@samsung.com>
+
+	* c-c++-common/asan/no-redundant-odr-indicators-1.c: New test.
+
 2016-12-01  Segher Boessenkool  <segher@kernel.crashing.org>
 
 	PR rtl-optimization/78607
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-odr-indicators-1.c b/gcc/testsuite/c-c++-common/asan/no-redundant-odr-indicators-1.c
new file mode 100644
index 0000000..ff1f272
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-odr-indicators-1.c
@@ -0,0 +1,17 @@ 
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+/* Local variables should not have odr indicators.  */
+static int a = 2;
+/* Thread local variables should not have odr indicators.  */
+__thread int b = 3;
+/* Externally visible  variables should have odr indicators.  */
+int c = 1;
+
+int main () {
+    return 0;
+}
+
+/* { dg-final { scan-assembler-not ".*odr_asan_a.*" } } */
+/* { dg-final { scan-assembler-not ".*odr_asan_b.*" } } */
+/* { dg-final { scan-assembler ".*odr_asan_c.*" } } */