Re: [PATCH] Implement -fsanitize=object-size

[Jakub Jelinek <jakub@redhat.com>]

On Sun, Jul 13, 2014 at 07:55:44PM +0200, Marek Polacek wrote:
> 2014-07-13  Marek Polacek  <polacek@redhat.com>
> 
> 	* ubsan.h (struct ubsan_mismatch_data):

Missing description.


> +/* Expand UBSAN_OBJECT_SIZE internal call.  */
> +
> +void
> +ubsan_expand_objsize_ifn (gimple_stmt_iterator *gsi)
> +{
> +  gimple stmt = gsi_stmt (*gsi);
> +  location_t loc = gimple_location (stmt);
> +  gcc_assert (gimple_call_num_args (stmt) == 4);
> +
> +  tree base = gimple_call_arg (stmt, 0);
> +  tree ptr = gimple_call_arg (stmt, 1);
> +  tree size = gimple_call_arg (stmt, 2);
> +  tree ckind = gimple_call_arg (stmt, 3);
> +  gimple_stmt_iterator gsi_orig = *gsi;
> +  gimple g;
> +
> +  gcc_assert (TREE_CODE (size) == INTEGER_CST);
> +  /* See if we can discard the check.  */
> +  if (tree_to_uhwi (size) == (unsigned HOST_WIDE_INT) -1)

This would be integer_all_onesp (size).

> +static void
> +instrument_object_size (gimple_stmt_iterator *gsi, bool is_lhs)
> +{
> +  gimple stmt = gsi_stmt (*gsi);
> +  location_t loc = gimple_location (stmt);
> +  tree t = is_lhs ? gimple_get_lhs (stmt) : gimple_assign_rhs1 (stmt);
> +
> +  if (TREE_CODE (t) != MEM_REF)
> +    return;

I think this is undesirable.  IMHO you want to call here
get_inner_reference, and if the given size is equal to maxsize, consider
instrumenting it, otherwise you don't instrument e.g. COMPONENT_REFs and
many other things.  Look at what e.g. asan.c or even ubsan.c does; the
question is what exactly to do with bitfields, but supposedly we should
require that the DECL_BIT_FIELD_REPRESENTATIVE is accessible in that case.

Also, I wonder if using base, ptr, objsz, ckind arguments are best for the
builtin, I'd think you want instead base, ptr+size-base, objsz, ckind.
Reasons:
a) the size addition when expanding UBSAN_OBJECT_SIZE will not work
   reliably, the middle end considers all pointer conversions useless,
   so you can very well end up with a different TREE_TYPE of the pointer
   type
b) sanopt runs very late, there aren't many GIMPLE optimization passes,
   so to optimize the condition checks you pretty much rely on RTL passes
c) for e.g. gimple_fold_call it will be much easier if it can remove
   redundant UBSAN_OBJECT_SIZE calls if it can just compare two constants

> +  tree ptr = TREE_OPERAND (t, 0);
> +  tree sizet, base = ptr;
> +  gimple g;
> +  gimple def_stmt;
> +
> +  while (TREE_CODE (base) == SSA_NAME)
> +    {
> +      def_stmt = SSA_NAME_DEF_STMT (base);
> +      if (is_gimple_assign (def_stmt))
> +	base = gimple_assign_rhs1 (def_stmt);

This looks too dangerous.  All you should look through are:
a) gimple_assign_ssa_name_copy_p
b) gimple_assign_cast_p if the rhs1 also has POINTER_TYPE_P
c) gimple_assign_rhs_code == POINTER_PLUS_EXPR

I'm also including a testcase, which shows why instrumenting
also COMPONENT_REFs etc. is important (see my reference to
get_inner_reference above) and also that IMHO we should instrument
not just when the base is a pointer, but also when it is a decl,
but in that case we should avoid instrumenting when -fsanitize=bounds
is on and we know it will handle it (in particular, if there was e.g.
char d[8]; int e; in the struct definition instead).

Note, the testcase ICEs with -O2 -fsanitize=bounds, can you please look
at that first and fix it separately?

struct S { int a; int b; };
struct T { int c; char d[]; };

static inline __attribute__((always_inline)) int
foo (struct S *p)
{
  return p->b;
}

int
bar (void)
{
  struct S *p = __builtin_calloc (sizeof (int) + sizeof (int) / 2, 1);
  return foo (p);
}

struct T t = { 1, "abcdefg" };

int
baz (int i)
{
  return t.d[i];
}

Other comments, in a form of a patch:
1) the gimple_fold_call bit shows that we should for the quite common
   case where __bos is folded into -1 remove the UBSAN_OBJECT_SIZE call
   immediately, not worth keeping it around through many other passes
2) if you add -O2 to the dg-options, that just means the tests are done
   8 times or how many with -O2 all the time.  Better skip it unless
   -O2
3) when the second argument is something that can be directly compared
   against the third argument, you can in gimple_fold_call fold not just
   the "don't know" cases, but also when the third argument is >= the
   second and both are INTEGER_CSTs - then we know at compile time
   we are ok.

--- gcc/gimple-fold.c.jj	2014-07-07 10:39:45.000000000 +0200
+++ gcc/gimple-fold.c	2014-07-14 12:41:15.419687543 +0200
@@ -1209,6 +1209,15 @@ gimple_fold_call (gimple_stmt_iterator *
 					gimple_call_arg (stmt, 1),
 					gimple_call_arg (stmt, 2));
 	  break;
+        case IFN_UBSAN_OBJECT_SIZE:
+	  if (integer_all_onesp (gimple_call_arg (stmt, 2)))
+	    {
+	      gsi_replace (gsi, gimple_build_nop (), true);
+	      unlink_stmt_vdef (stmt);
+	      release_defs (stmt);
+	      return true;
+	    }
+	  break;
 	case IFN_UBSAN_CHECK_ADD:
 	  subcode = PLUS_EXPR;
 	  break;
--- gcc/testsuite/c-c++-common/ubsan/object-size-3.c.jj	2014-07-14 10:42:24.000000000 +0200
+++ gcc/testsuite/c-c++-common/ubsan/object-size-3.c	2014-07-14 12:08:00.379413090 +0200
@@ -1,5 +1,6 @@
 /* { dg-do run } */
-/* { dg-options "-fsanitize=object-size -O2 -fno-sanitize-recover" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */
+/* { dg-options "-fsanitize=object-size -fno-sanitize-recover" } */
 
 /* Test valid uses.  */
 
--- gcc/testsuite/c-c++-common/ubsan/object-size-2.c.jj	2014-07-14 10:42:24.000000000 +0200
+++ gcc/testsuite/c-c++-common/ubsan/object-size-2.c	2014-07-14 12:07:54.760440751 +0200
@@ -1,5 +1,6 @@
 /* { dg-do compile } */
-/* { dg-options "-fsanitize=undefined -O2" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */
+/* { dg-options "-fsanitize=undefined" } */
 
 void
 foo (unsigned long ul)
--- gcc/testsuite/c-c++-common/ubsan/object-size-6.c.jj	2014-07-14 10:42:24.000000000 +0200
+++ gcc/testsuite/c-c++-common/ubsan/object-size-6.c	2014-07-14 12:08:21.063305726 +0200
@@ -1,5 +1,6 @@
 /* { dg-do compile } */
-/* { dg-options "-fsanitize=object-size -O2" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */
+/* { dg-options "-fsanitize=object-size" } */
 
 char
 foo (void *v)
--- gcc/testsuite/c-c++-common/ubsan/object-size-4.c.jj	2014-07-14 10:42:24.000000000 +0200
+++ gcc/testsuite/c-c++-common/ubsan/object-size-4.c	2014-07-14 12:08:05.867384036 +0200
@@ -1,5 +1,6 @@
 /* { dg-do run } */
-/* { dg-options "-fsanitize=object-size -O2 -fno-sanitize-recover" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */
+/* { dg-options "-fsanitize=object-size -fno-sanitize-recover" } */
 
 /* Test that we don't instrument flexible array members.  */
 
--- gcc/testsuite/c-c++-common/ubsan/object-size-1.c.jj	2014-07-14 10:42:24.000000000 +0200
+++ gcc/testsuite/c-c++-common/ubsan/object-size-1.c	2014-07-14 12:07:49.463467643 +0200
@@ -1,5 +1,6 @@
 /* { dg-do run } */
-/* { dg-options "-fsanitize=undefined -O2" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */
+/* { dg-options "-fsanitize=undefined" } */
 
 /* Sanity-test -fsanitize=object-size.  We use -fsanitize=undefined option
    to check that this feature doesn't clash with -fsanitize=bounds et al.  */
--- gcc/testsuite/c-c++-common/ubsan/object-size-7.c.jj	2014-07-14 10:42:24.000000000 +0200
+++ gcc/testsuite/c-c++-common/ubsan/object-size-7.c	2014-07-14 12:08:26.685276937 +0200
@@ -1,5 +1,6 @@
 /* { dg-do run } */
-/* { dg-options "-fsanitize=object-size -O2" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */
+/* { dg-options "-fsanitize=object-size" } */
 
 #define N 20
 
--- gcc/testsuite/c-c++-common/ubsan/object-size-5.c.jj	2014-07-14 10:42:24.000000000 +0200
+++ gcc/testsuite/c-c++-common/ubsan/object-size-5.c	2014-07-14 12:08:10.901364754 +0200
@@ -1,5 +1,6 @@
 /* { dg-do run } */
-/* { dg-options "-fsanitize=object-size -O2" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */
+/* { dg-options "-fsanitize=object-size" } */
 
 /* Test structures with -fsanitize=object-size.  */
 

	Jakub