Re: [PATCH] Don't always instrument shifts (PR sanitizer/58413)

[Jakub Jelinek <jakub at redhat dot com>]

On Fri, Sep 13, 2013 at 08:01:36PM +0200, Marek Polacek wrote:
> 2013-09-13  Marek Polacek  <polacek@redhat.com>
> 
> 	PR sanitizer/58413
> c-family/
> 	* c-ubsan.c (ubsan_instrument_shift): Don't instrument
> 	an expression if we can prove it is correct.
> 
> testsuite/
> 	* c-c++-common/ubsan/shift-4.c: New test.
> 	* c-c++-common/ubsan/shift-5.c: New test.
> 	* gcc.dg/ubsan/c-shift-1.c: New test.
> 
> --- gcc/c-family/c-ubsan.c.mp3	2013-09-13 19:19:55.410925155 +0200
> +++ gcc/c-family/c-ubsan.c	2013-09-13 19:20:16.766006242 +0200
> @@ -104,6 +104,40 @@ ubsan_instrument_shift (location_t loc,
>    tree uprecm1 = build_int_cst (op1_utype, op0_prec - 1);
>    tree precm1 = build_int_cst (type1, op0_prec - 1);
>  
> +  /* If OP0 is of an unsigned type, we may prove that OP1 is not
> +     greater than UPRECM1 (and not negative); in that case we can
> +     bail out early.  */
> +  if (TYPE_UNSIGNED (type0)
> +      && TREE_CODE (op1) == INTEGER_CST
> +      && tree_int_cst_sgn (op1) != -1
> +      && !tree_int_cst_lt (uprecm1, op1))
> +    return NULL_TREE;
> +
> +  /* Even when OP0 is of a signed type, we may prove that there's
> +     nothing wrong with the shift if both operands are INTEGER_CSTs
> +     and wouldn't trigger UB.  We do this only for C.
> +     XXX Should we treat ANSI C specially wrt 1 << 31?  */
> +  if (TREE_CODE (op0) == INTEGER_CST
> +      && TREE_CODE (op1) == INTEGER_CST
> +      && !TYPE_UNSIGNED (type0)
> +      && tree_int_cst_sgn (op1) != -1
> +      && !tree_int_cst_lt (uprecm1, op1)
> +      && !c_dialect_cxx ())
> +    {
> +      /* If this is a right shift, we can return now.  */
> +      if (code == RSHIFT_EXPR)
> +        return NULL_TREE;
> +
> +      /* Otherwise see comment below.  */
> +      tree x = fold_convert_loc (loc, unsigned_type_for (type0), op0);
> +      x = fold_build2 (RSHIFT_EXPR, TREE_TYPE (x), x,
> +		       fold_build2 (MINUS_EXPR, TREE_TYPE (precm1), precm1,
> +				    op1));
> +      if (integer_zerop (x))
> +        return NULL_TREE;
> +    }
> +
> +  /* Ok, we have to do the instrumentation.  */

I'd say the above is going to be a maintainance nightmare, with all the code
duplication.  And you are certainly going to miss cases that way,
e.g.
void
foo (void)
{
  int A[-2 / -1] = {};
}

I'd say instead of adding all this, you should just at the right spot insert
if (integer_zerop (t)) return NULL_TREE; or similar.

For shift instrumentation, I guess you could add
if (integer_zerop (t) && (tt == NULL_TREE || integer_zerop (tt)))
  return NULL_TREE;
right before:
  t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);

> +/* PR sanitizer/58413 */
> +/* { dg-do run } */
> +/* { dg-options "-fsanitize=shift -w" } */
> +
> +int x = 7;
> +int
> +main (void)
> +{
> +  /* All of the following should pass.  */
> +  int A[128 >> 5] = {};
> +  int B[128 << 5] = {};
> +
> +  static int e =
> +    ((int)
> +     (0x00000000 | ((31 & ((1 << (4)) - 1)) << (((15) + 6) + 4)) |
> +      ((0) << ((15) + 6)) | ((0) << (15))));

This relies on int32plus, so needs to be /* { dg-do run { target int32plus } } */
> --- gcc/testsuite/c-c++-common/ubsan/shift-5.c.mp3	2013-09-13 18:25:06.195847144 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/shift-5.c	2013-09-13 19:16:38.990211229 +0200
> @@ -0,0 +1,21 @@
> +/* { dg-do compile} */
> +/* { dg-options "-fsanitize=shift -w" } */
> +/* { dg-shouldfail "ubsan" } */
> +
> +int x;
> +int
> +main (void)
> +{
> +  /* None of the following should pass.  */
> +  switch (x)
> +    {
> +    case 1 >> -1:	/* { dg-error "" } */
> +    case -1 >> -1:	/* { dg-error "" } */
> +    case 1 << -1:	/* { dg-error "" } */
> +    case -1 << -1:	/* { dg-error "" } */
> +    case -1 >> 200:	/* { dg-error "" } */
> +    case 1 << 200:	/* { dg-error "" } */

Can't you fill in the error you are expecting, or is the problem
that the wording is very different between C and C++?

	Jakub