Re: [PATCH] [asan] Fix for PR asan/56330

[Jack Howarth <howarth at bromo dot med dot uc dot edu>]

On Fri, Feb 15, 2013 at 09:54:19PM +0100, Dodji Seketeli wrote:
> Hello,
> 
> This PR uncovers an issue my latest optimization patch on Address
> Sanitizer introduced.
> 
> This is in the context of instrumenting an access to a memory region,
> like in:
> 
>     void
>     foo (char *a, char *b, int s)
>     {
>       __builtin_memmove (a, b, s);
>     }
> 
> In this case, asan has to instrument accesses to two memory regions:
> [a a+s[ and [b b+s[.
> 
> The way asan does instrument the access to e.g [a a+s[ his is that it
> instruments the access to the byte pointed to by the pointer 'a', and
> the access to the byte pointed to by the pointer 'a+s-1'.
> 
> Now what happens when we want to avoid doing redundant
> instrumentations like in:
> 
>     void
>     bar (char *a, char *b, char *c)
>     {
>       __builtin_memmove (a, b, c[b[0]]);
>     }
> 
> So here, we first instrument the access to the memory of b[0] (in the
> expression c[b[0]]).  So in the course of instrumenting the access for
> the memory region [b b+s[, we don't want to instrument the access to
> the memory pointed by 'p', as that was already been instrumented for
> the b[0] expression.  So we just instrument the access to memory
> through b+s-1.
> 
> This is what I'd call 'partial memory region instrumentation'; it
> happens in the function instrument_mem_region_access, and it was
> basically corrupting the CFG because instrument_mem_region_access was
> not correctly updating the statement iterator it receives in argument
> so subsequent use of that iterator (to append instrumentation
> statements) was leading to statements not being added to their correct
> place.  The compiler crash reported in this PR is due to this CFG
> corruption.
> 
> There is another underlying issue that can lead to runtime errors.
> When doing partial memory region instrumentation in e.g the case of
> the bar function above I was forgetting to enclose the instrumentation
> statements of [a, a+s[ and [b, b+s[ into a if (len != 0) {} clause
> (len being c[b[0]] here) for the cases where len is not a constant.
> The patch addresses this too.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk, and
> approved by Jakub in the audit trail of the PR at
> http://gcc.gnu.org/PR56330.  I am about to commit in a short while.

On x86_64-apple-darwin12, this patch is producing a failure of...

FAIL: c-c++-common/asan/no-redundant-instrumentation-7.c  -O0   scan-tree-dump-times asan0 "__builtin___asan_report_load" 6

at both -m32 and -m64. The no-redundant-instrumentation-7.s from the failing -m64
test case is attached, generated with...

# /sw/src/fink.build/gcc48-4.8.0-1000/darwin_objdir/gcc/testsuite/g++/../../xg++ -B/sw/src/fink.build/gcc48-4.8.0-1000/darwin_objdir/gcc/testsuite/g++/../../ /sw/src/fink.build/gcc48-4.8.0-1000/gcc-4.8-20130215/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c  -fsanitize=address -g -fno-diagnostics-show-caret  -nostdinc++ -I/sw/src/fink.build/gcc48-4.8.0-1000/darwin_objdir/x86_64-apple-darwin12.2.0/libstdc++-v3/include/x86_64-apple-darwin12.2.0 -I/sw/src/fink.build/gcc48-4.8.0-1000/darwin_objdir/x86_64-apple-darwin12.2.0/libstdc++-v3/include -I/sw/src/fink.build/gcc48-4.8.0-1000/gcc-4.8-20130215/libstdc++-v3/libsupc++ -I/sw/src/fink.build/gcc48-4.8.0-1000/gcc-4.8-20130215/libstdc++-v3/include/backward -I/sw/src/fink.build/gcc48-4.8.0-1000/gcc-4.8-20130215/libstdc++-v3/testsuite/util -fmessage-length=0  -O0  -fdump-tree-asan0  -S  -m64 -o no-redundant-instrumentation-7.s

> 
> gcc/
> 	* asan.c (get_mem_refs_of_builtin_call): White space and style
> 	cleanup.
> 	(instrument_mem_region_access): Do not forget to always put
> 	instrumentation of the of 'base' and 'base + len' in a "if (len !=
> 	0) statement, even for cases where either 'base' or 'base + len'
> 	are not instrumented -- because they have been previously
> 	instrumented.  Simplify the logic by putting all the statements
> 	instrument 'base + len' inside a sequence, and then insert that
> 	sequence right before the current insertion point.  Then, to
> 	instrument 'base + len', just get an iterator on that statement.
> 	And do not forget to update the pointer to iterator the function
> 	received as argument.
> 
> gcc/testsuite/
> 
> 	* c-c++-common/asan/no-redundant-instrumentation-4.c: New test file.
> 	* c-c++-common/asan/no-redundant-instrumentation-5.c: Likewise.
> 	* c-c++-common/asan/no-redundant-instrumentation-6.c: Likewise.
> 	* c-c++-common/asan/no-redundant-instrumentation-7.c: Likewise.
> 	* c-c++-common/asan/no-redundant-instrumentation-8.c: Likewise.
> 	* c-c++-common/asan/pr56330.c: Likewise.
> 	* c-c++-common/asan/no-redundant-instrumentation-1.c (test1):
> 	Ensure the size argument of __builtin_memcpy is a constant.
> ---
>  gcc/asan.c                                         | 97 ++++++++++++----------
>  .../asan/no-redundant-instrumentation-1.c          |  2 +-
>  .../asan/no-redundant-instrumentation-4.c          | 13 +++
>  .../asan/no-redundant-instrumentation-5.c          | 13 +++
>  .../asan/no-redundant-instrumentation-6.c          | 14 ++++
>  .../asan/no-redundant-instrumentation-7.c          | 22 +++++
>  .../asan/no-redundant-instrumentation-8.c          | 14 ++++
>  gcc/testsuite/c-c++-common/asan/pr56330.c          | 23 +++++
>  8 files changed, 153 insertions(+), 45 deletions(-)
>  create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c
>  create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c
>  create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c
>  create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c
>  create mode 100644 gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c
>  create mode 100644 gcc/testsuite/c-c++-common/asan/pr56330.c
> 
> diff --git a/gcc/asan.c b/gcc/asan.c
> index a569479..67236a9 100644
> --- a/gcc/asan.c
> +++ b/gcc/asan.c
> @@ -747,20 +747,17 @@ get_mem_refs_of_builtin_call (const gimple call,
>  
>        got_reference_p = true;
>      }
> -    else
> -      {
> -	if (dest)
> -	  {
> -	    dst->start = dest;
> -	    dst->access_size = access_size;
> -	    *dst_len = NULL_TREE;
> -	    *dst_is_store = is_store;
> -	    *dest_is_deref = true;
> -	    got_reference_p = true;
> -	  }
> -      }
> +  else if (dest)
> +    {
> +      dst->start = dest;
> +      dst->access_size = access_size;
> +      *dst_len = NULL_TREE;
> +      *dst_is_store = is_store;
> +      *dest_is_deref = true;
> +      got_reference_p = true;
> +    }
>  
> -    return got_reference_p;
> +  return got_reference_p;
>  }
>  
>  /* Return true iff a given gimple statement has been instrumented.
> @@ -1535,8 +1532,15 @@ instrument_mem_region_access (tree base, tree len,
>  
>    /* If the beginning of the memory region has already been
>       instrumented, do not instrument it.  */
> -  if (has_mem_ref_been_instrumented (base, 1))
> -    goto after_first_instrumentation;
> +  bool start_instrumented = has_mem_ref_been_instrumented (base, 1);
> +
> +  /* If the end of the memory region has already been instrumented, do
> +     not instrument it. */
> +  tree end = asan_mem_ref_get_end (base, len);
> +  bool end_instrumented = has_mem_ref_been_instrumented (end, 1);
> +
> +  if (start_instrumented && end_instrumented)
> +    return;
>  
>    if (!is_gimple_constant (len))
>      {
> @@ -1562,37 +1566,39 @@ instrument_mem_region_access (tree base, tree len,
>  
>        /* The 'then block' of the 'if (len != 0) condition is where
>  	 we'll generate the asan instrumentation code now.  */
> -      gsi = gsi_start_bb (then_bb);
> +      gsi = gsi_last_bb (then_bb);
>      }
>  
> -  /* Instrument the beginning of the memory region to be accessed,
> -     and arrange for the rest of the intrumentation code to be
> -     inserted in the then block *after* the current gsi.  */
> -  build_check_stmt (location, base, &gsi, /*before_p=*/true, is_store, 1);
> -
> -  if (then_bb)
> -    /* We are in the case where the length of the region is not
> -       constant; so instrumentation code is being generated in the
> -       'then block' of the 'if (len != 0) condition.  Let's arrange
> -       for the subsequent instrumentation statements to go in the
> -       'then block'.  */
> -    gsi = gsi_last_bb (then_bb);
> -  else
> -    *iter = gsi;
> -
> -  update_mem_ref_hash_table (base, 1);
> +  if (!start_instrumented)
> +    {
> +      /* Instrument the beginning of the memory region to be accessed,
> +	 and arrange for the rest of the intrumentation code to be
> +	 inserted in the then block *after* the current gsi.  */
> +      build_check_stmt (location, base, &gsi, /*before_p=*/true, is_store, 1);
> +
> +      if (then_bb)
> +	/* We are in the case where the length of the region is not
> +	   constant; so instrumentation code is being generated in the
> +	   'then block' of the 'if (len != 0) condition.  Let's arrange
> +	   for the subsequent instrumentation statements to go in the
> +	   'then block'.  */
> +	gsi = gsi_last_bb (then_bb);
> +      else
> +        {
> +          *iter = gsi;
> +	  /* Don't remember this access as instrumented, if length
> +	     is unknown.  It might be zero and not being actually
> +	     instrumented, so we can't rely on it being instrumented.  */
> +          update_mem_ref_hash_table (base, 1);
> +	}
> +    }
>  
> - after_first_instrumentation:
> +  if (end_instrumented)
> +    return;
>  
>    /* We want to instrument the access at the end of the memory region,
>       which is at (base + len - 1).  */
>  
> -  /* If the end of the memory region has already been instrumented, do
> -     not instrument it. */
> -  tree end = asan_mem_ref_get_end (base, len);
> -  if (has_mem_ref_been_instrumented (end, 1))
> -    return;
> -
>    /* offset = len - 1;  */
>    len = unshare_expr (len);
>    tree offset;
> @@ -1639,8 +1645,6 @@ instrument_mem_region_access (tree base, tree len,
>  				  base, NULL);
>    gimple_set_location (region_end, location);
>    gimple_seq_add_stmt_without_update (&seq, region_end);
> -  gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
> -  gsi_prev (&gsi);
>  
>    /* _2 = _1 + offset;  */
>    region_end =
> @@ -1649,13 +1653,18 @@ instrument_mem_region_access (tree base, tree len,
>  				  gimple_assign_lhs (region_end),
>  				  offset);
>    gimple_set_location (region_end, location);
> -  gsi_insert_after (&gsi, region_end, GSI_NEW_STMT);
> +  gimple_seq_add_stmt_without_update (&seq, region_end);
> +  gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
>  
>    /* instrument access at _2;  */
> +  gsi = gsi_for_stmt (region_end);
>    build_check_stmt (location, gimple_assign_lhs (region_end),
>  		    &gsi, /*before_p=*/false, is_store, 1);
>  
> -  update_mem_ref_hash_table (end, 1);
> +  if (then_bb == NULL)
> +    update_mem_ref_hash_table (end, 1);
> +
> +  *iter = gsi_for_stmt (gsi_stmt (*iter));
>  }
>  
>  /* Instrument the call (to the builtin strlen function) pointed to by
> @@ -1783,7 +1792,7 @@ instrument_builtin_call (gimple_stmt_iterator *iter)
>  	    }
>  	  else if (src0_len || src1_len || dest_len)
>  	    {
> -	      if (src0.start)
> +	      if (src0.start != NULL_TREE)
>  		instrument_mem_region_access (src0.start, src0_len,
>  					      iter, loc, /*is_store=*/false);
>  	      if (src1.start != NULL_TREE)
> diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c
> index cc98fdb..6cf6441 100644
> --- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c
> +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c
> @@ -45,7 +45,7 @@ test1 ()
>    /* There are 2 calls to __builtin___asan_report_store1 and 2 calls
>       to __builtin___asan_report_load1 to instrument the store to
>       (subset of) the memory region of tab.  */
> -  __builtin_memcpy (&tab[1], foo, sizeof (tab) - 1);
> +  __builtin_memcpy (&tab[1], foo, 3);
>  
>    /* This should not generate a __builtin___asan_report_load1 because
>       the reference to tab[1] has been already instrumented above.  */
> diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c
> new file mode 100644
> index 0000000..b2e7284
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c
> @@ -0,0 +1,13 @@
> +/* { dg-options "-fdump-tree-asan0" } */
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
> +
> +void
> +foo  (int *a, char *b, char *c)
> +{
> +  __builtin_memmove (c, b, a[c[0]]);
> +}
> +
> +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 3 "asan0" } } */
> +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 1 "asan0" } } */
> +/* { dg-final { cleanup-tree-dump "asan0" } } */
> diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c
> new file mode 100644
> index 0000000..ead3f58
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c
> @@ -0,0 +1,13 @@
> +/* { dg-options "-fdump-tree-asan0" } */
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
> +
> +void
> +foo  (int *a, char *b, char *c)
> +{
> +  __builtin_memmove (c, b, a[b[0]]);
> +}
> +
> +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 2 "asan0" } } */
> +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 2 "asan0" } } */
> +/* { dg-final { cleanup-tree-dump "asan0" } } */
> diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c
> new file mode 100644
> index 0000000..e4691bc
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c
> @@ -0,0 +1,14 @@
> +/* { dg-options "-fdump-tree-asan0" } */
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
> +
> +void
> +foo  (int *a, char *b, char *c)
> +{
> +  __builtin_memmove (c, b, a[c[0]]);
> +  __builtin_memmove (c, b, a[b[0]]);
> +}
> +
> +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 5 "asan0" } } */
> +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 2 "asan0" } } */
> +/* { dg-final { cleanup-tree-dump "asan0" } } */
> diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c
> new file mode 100644
> index 0000000..aba409b
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c
> @@ -0,0 +1,22 @@
> +/* { dg-options "-fdump-tree-asan0" } */
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
> +
> +char e[200];
> +
> +struct S
> +{
> +  char a[100];
> +  char b[100];
> +} s;
> +
> +void
> +foo  (int *a, char *b, char *c)
> +{
> +  __builtin_memcmp (s.a, e, 100);
> +  __builtin_memcmp (s.a, e, 200);
> +}
> +
> +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load" 6 "asan0" } } */
> +/* { dg-final { scan-tree-dump-not "__builtin___asan_report_store" "asan0" } } */
> +/* { dg-final { cleanup-tree-dump "asan" } } */
> diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c
> new file mode 100644
> index 0000000..38ea7a2
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c
> @@ -0,0 +1,14 @@
> +/* { dg-options "-fdump-tree-asan0" } */
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
> +
> +char
> +foo  (int *a, char *b, char *c)
> +{
> +  __builtin_memmove (c, b, a[b[0]]);
> +  return c[0] + b[0];
> +}
> +
> +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 3 "asan0" } } */
> +/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 2 "asan0" } } */
> +/* { dg-final { cleanup-tree-dump "asan0" } } */
> diff --git a/gcc/testsuite/c-c++-common/asan/pr56330.c b/gcc/testsuite/c-c++-common/asan/pr56330.c
> new file mode 100644
> index 0000000..18f1065
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/pr56330.c
> @@ -0,0 +1,23 @@
> +/* PR sanitizer/56330 */
> +/* { dg-do compile } */
> +
> +char e[200];
> +
> +struct S
> +{
> +  char a[100];
> +  char b[100];
> +} s;
> +
> +void
> +foo (void)
> +{
> +  __builtin_memcmp (s.a, e, 100);
> +  __builtin_memcmp (s.a, e, 200);
> +}
> +
> +void
> +bar (int *a, char *b, char *c)
> +{
> +  __builtin_memmove (c, b, a[b[0]]);
> +}
> -- 
> 		Dodji

Attachment: no-redundant-instrumentation-7.s
Description: Text document