[ubsan] Fix uninitialized var issue (PR sanitizer/64906)

This fixes sanitizer/64906 by also pre-evaluating OP1.  The problem
in this testcase is that OP1 is "SAVE_EXPR <f ? 8 : 0>" which can't
be folded any more, but when we're creating the EQ_EXPR -- checking
whether this expression equals zero -- it can be folded further to
"f == 0".  Afterwards we end up with this (crap omitted):

      ret = if (SAVE_EXPR <(long unsigned int) s>;, SAVE_EXPR <f == 0>;)
        {
          __ubsan (..., (SAVE_EXPR <f != 0 ? 8 : 0>)); 
        }
      else
        {
        }, (... / (SAVE_EXPR <f != 0 ? 8 : 0>));

I.e. the SAVE_EXPR <f != 0 ? 8 : 0> would be evaluated inside the if and not
before.  So do what we already do for OP0.

The shift instrumentation should be fine in this regard.

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

2015-09-22  Marek Polacek  <polacek@redhat.com>

	PR sanitizer/64906
	* c-ubsan.c (ubsan_instrument_division): Also pre-evaluate OP1.

	* c-c++-common/ubsan/pr64906.c: New test.


	Marek

On 09/22/2015 05:11 PM, Marek Polacek wrote:

> diff --git gcc/c-family/c-ubsan.c gcc/c-family/c-ubsan.c
> index e0cce84..d2bc264 100644
> --- gcc/c-family/c-ubsan.c
> +++ gcc/c-family/c-ubsan.c
> @@ -104,6 +104,7 @@ ubsan_instrument_division (location_t loc, tree op0, tree op1)
>   	}
>       }
>     t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), unshare_expr (op0), t);
> +  t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), unshare_expr (op1), t);
>     if (flag_sanitize_undefined_trap_on_error)
>       tt = build_call_expr_loc (loc, builtin_decl_explicit (BUILT_IN_TRAP), 0);
>     else

I really don't know this code, but just before the location you're 
patching, there's this:

   /* In case we have a SAVE_EXPR in a conditional context, we need to
      make sure it gets evaluated before the condition.  If the OP0 is
      an instrumented array reference, mark it as having side effects so
      it's not folded away.  */
   if (flag_sanitize & SANITIZE_BOUNDS)
     {
       tree xop0 = op0;
       while (CONVERT_EXPR_P (xop0))
         xop0 = TREE_OPERAND (xop0, 0);
       if (TREE_CODE (xop0) == ARRAY_REF)
         {
           TREE_SIDE_EFFECTS (xop0) = 1;
           TREE_SIDE_EFFECTS (op0) = 1;
         }
     }

Does that need to be done for op1 as well? (I really wonder why this is 
needed or whether it's sufficient to find such an ARRAY_REF if you can 
have more complex operands).

The same pattern occurs in another function, so it may be best to break 
it out into a new function if additional occurrences are necessary.


Bernd

On Wed, Sep 23, 2015 at 01:08:53PM +0200, Bernd Schmidt wrote:
> On 09/22/2015 05:11 PM, Marek Polacek wrote:
> 
> >diff --git gcc/c-family/c-ubsan.c gcc/c-family/c-ubsan.c
> >index e0cce84..d2bc264 100644
> >--- gcc/c-family/c-ubsan.c
> >+++ gcc/c-family/c-ubsan.c
> >@@ -104,6 +104,7 @@ ubsan_instrument_division (location_t loc, tree op0, tree op1)
> >  	}
> >      }
> >    t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), unshare_expr (op0), t);
> >+  t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), unshare_expr (op1), t);
> >    if (flag_sanitize_undefined_trap_on_error)
> >      tt = build_call_expr_loc (loc, builtin_decl_explicit (BUILT_IN_TRAP), 0);
> >    else
> 
> I really don't know this code, but just before the location you're patching,
> there's this:
> 
>   /* In case we have a SAVE_EXPR in a conditional context, we need to
>      make sure it gets evaluated before the condition.  If the OP0 is
>      an instrumented array reference, mark it as having side effects so
>      it's not folded away.  */
>   if (flag_sanitize & SANITIZE_BOUNDS)
>     {
>       tree xop0 = op0;
>       while (CONVERT_EXPR_P (xop0))
>         xop0 = TREE_OPERAND (xop0, 0);
>       if (TREE_CODE (xop0) == ARRAY_REF)
>         {
>           TREE_SIDE_EFFECTS (xop0) = 1;
>           TREE_SIDE_EFFECTS (op0) = 1;
>         }
>     }
> 
> Does that need to be done for op1 as well? (I really wonder why this is
> needed or whether it's sufficient to find such an ARRAY_REF if you can have
> more complex operands).
 
Good point.  I've dug into this and that hunk doesn't seem to be needed
(anymore?).  I suppose there was a reason I added that, but removing it
doesn't seem to break anything.  It can be triggered with a code like:

struct S
{
  unsigned long a[1];
  int l;
};

static inline unsigned long
fn (const struct S *s, int i)
{
  return s->a[i] / i;
}

int
main ()
{
  struct S s;
  fn (&s, 1);
}

With the hunk, we sanitize the same array twice -- that's "suboptimal".  With
the hunk removed, we sanitize the array just once as expected.

> The same pattern occurs in another function, so it may be best to break it
> out into a new function if additional occurrences are necessary.

Given that the code above seems to be useless now, I think let's put this
patch in as-is, backport it to gcc-5, then remove those redundant hunks on
trunk and add the testcase above.  Do you agree?

	Marek

On 09/23/2015 06:07 PM, Marek Polacek wrote:
> Given that the code above seems to be useless now, I think let's put this
> patch in as-is, backport it to gcc-5, then remove those redundant hunks on
> trunk and add the testcase above.  Do you agree?

Sounds reasonable. If you can find a point in the history where that 
code wasn't useless, it would be good to help us understand why it's there.


Bernd

On Wed, Sep 23, 2015 at 08:55:53PM +0200, Bernd Schmidt wrote:
> On 09/23/2015 06:07 PM, Marek Polacek wrote:
> >Given that the code above seems to be useless now, I think let's put this
> >patch in as-is, backport it to gcc-5, then remove those redundant hunks on
> >trunk and add the testcase above.  Do you agree?
> 
> Sounds reasonable. If you can find a point in the history where that code
> wasn't useless, it would be good to help us understand why it's there.

I did some archeology.  The code wasn't useless since it was added (r211859)
till r226110 where I added some unshare_exprs.  On the testcase I posted
earlier in the thread that makes a difference:

@@ -11,7 +11,7 @@
   else
     {
       <<< Unknown tree: void_cst >>>
-    }, (long unsigned int) (s->a[i] << SAVE_EXPR <i>);;
+    }, (long unsigned int) (s->a[UBSAN_BOUNDS (0B, SAVE_EXPR <i>, 0);,
SAVE_EXPR <i>;] << SAVE_EXPR <i>);;
 }

So we instrument the array multiple times as it's not shared anymore.

Ok to proceed with the plan I mentioned above?

	Marek

On 09/24/2015 11:32 AM, Marek Polacek wrote:
> On Wed, Sep 23, 2015 at 08:55:53PM +0200, Bernd Schmidt wrote:
>> On 09/23/2015 06:07 PM, Marek Polacek wrote:
>>> Given that the code above seems to be useless now, I think let's put this
>>> patch in as-is, backport it to gcc-5, then remove those redundant hunks on
>>> trunk and add the testcase above.  Do you agree?
>>
>> Sounds reasonable. If you can find a point in the history where that code
>> wasn't useless, it would be good to help us understand why it's there.
>
> I did some archeology.  The code wasn't useless since it was added (r211859)
> till r226110 where I added some unshare_exprs.  On the testcase I posted
> earlier in the thread that makes a difference:
>
> @@ -11,7 +11,7 @@
>     else
>       {
>         <<< Unknown tree: void_cst >>>
> -    }, (long unsigned int) (s->a[i] << SAVE_EXPR <i>);;
> +    }, (long unsigned int) (s->a[UBSAN_BOUNDS (0B, SAVE_EXPR <i>, 0);,
> SAVE_EXPR <i>;] << SAVE_EXPR <i>);;
>   }
>
> So we instrument the array multiple times as it's not shared anymore.
>
> Ok to proceed with the plan I mentioned above?

Yes.


Bernd