[ubsan PATCH] Fix ICE with bounds checking on VLA-in-a-struct (PR sanitizer/70875)

[ubsan PATCH] Fix ICE with bounds checking on VLA-in-a-struct (PR sanitizer/70875)

[Marek Polacek]

A program containing an array of structs containing a VLA caused ICE with UBSAN
bounds checking, because in get_ubsan_type_info_for_type we asserted that the
size of a type fits uhwi, which implies it is an INTEGER_CST.  But that's not
the case for a struct with VLA.  However, the assert here is bogus, for
!REAL_TYPE and !INTEGRAL_TYPE_P get_ubsan_type_info_for_type just returns 0.
And since tree_to_uhwi has
  gcc_assert (tree_fits_uhwi_p (t));
there's no need to duplicate that for the REAL_TYPE / INTEGRAL_TYPE_P cases.

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

2016-05-06  Marek Polacek  <polacek@redhat.com>

	PR sanitizer/70875
	* ubsan.c (get_ubsan_type_info_for_type): Remove assert.

	* gcc.dg/ubsan/bounds-3.c: New test.

diff --git gcc/testsuite/gcc.dg/ubsan/bounds-3.c gcc/testsuite/gcc.dg/ubsan/bounds-3.c
index e69de29..50ad673 100644
--- gcc/testsuite/gcc.dg/ubsan/bounds-3.c
+++ gcc/testsuite/gcc.dg/ubsan/bounds-3.c
@@ -0,0 +1,22 @@
+/* PR sanitizer/70875 */
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds" } */
+
+int
+foo (int n, int k)
+{
+  struct S
+  {
+    int i[n];
+    int value;
+  } s[2];
+  return s[k].value = 0;
+}
+
+int
+main ()
+{
+  return foo (2, 2);
+}
+
+/* { dg-output "index 2 out of bounds for type 'S \\\[2\\\]'" } */
diff --git gcc/ubsan.c gcc/ubsan.c
index 802341e..c5543f8 100644
--- gcc/ubsan.c
+++ gcc/ubsan.c
@@ -302,7 +302,6 @@ ubsan_source_location (location_t loc)
 static unsigned short
 get_ubsan_type_info_for_type (tree type)
 {
-  gcc_assert (TYPE_SIZE (type) && tree_fits_uhwi_p (TYPE_SIZE (type)));
   if (TREE_CODE (type) == REAL_TYPE)
     return tree_to_uhwi (TYPE_SIZE (type));
   else if (INTEGRAL_TYPE_P (type))

	Marek

Re: [ubsan PATCH] Fix ICE with bounds checking on VLA-in-a-struct (PR sanitizer/70875)

[Jakub Jelinek]

On Fri, May 06, 2016 at 11:22:41AM +0200, Marek Polacek wrote:
> A program containing an array of structs containing a VLA caused ICE with UBSAN
> bounds checking, because in get_ubsan_type_info_for_type we asserted that the
> size of a type fits uhwi, which implies it is an INTEGER_CST.  But that's not
> the case for a struct with VLA.  However, the assert here is bogus, for
> !REAL_TYPE and !INTEGRAL_TYPE_P get_ubsan_type_info_for_type just returns 0.
> And since tree_to_uhwi has
>   gcc_assert (tree_fits_uhwi_p (t));
> there's no need to duplicate that for the REAL_TYPE / INTEGRAL_TYPE_P cases.

Yeah, and for NULL TYPE_SIZE we just segfault, not really need to assert
that.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

Ok, thanks.  If it affects 6.x branch, it is ok there as well.

> 2016-05-06  Marek Polacek  <polacek@redhat.com>
> 
> 	PR sanitizer/70875
> 	* ubsan.c (get_ubsan_type_info_for_type): Remove assert.
> 
> 	* gcc.dg/ubsan/bounds-3.c: New test.

	Jakub

Re: [ubsan PATCH] Fix ICE with bounds checking on VLA-in-a-struct (PR sanitizer/70875)

[Marek Polacek]

On Fri, May 06, 2016 at 11:29:33AM +0200, Jakub Jelinek wrote:
> On Fri, May 06, 2016 at 11:22:41AM +0200, Marek Polacek wrote:
> > A program containing an array of structs containing a VLA caused ICE with UBSAN
> > bounds checking, because in get_ubsan_type_info_for_type we asserted that the
> > size of a type fits uhwi, which implies it is an INTEGER_CST.  But that's not
> > the case for a struct with VLA.  However, the assert here is bogus, for
> > !REAL_TYPE and !INTEGRAL_TYPE_P get_ubsan_type_info_for_type just returns 0.
> > And since tree_to_uhwi has
> >   gcc_assert (tree_fits_uhwi_p (t));
> > there's no need to duplicate that for the REAL_TYPE / INTEGRAL_TYPE_P cases.
> 
> Yeah, and for NULL TYPE_SIZE we just segfault, not really need to assert
> that.
> > 
> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> Ok, thanks.  If it affects 6.x branch, it is ok there as well.

Yeah, it does, and since I need to test a backport for PR70342 anyway,
I'll add it to my testing & commit it afterwards.

Thanks,

	Marek