diff mbox series

RFC [PATCH] c: Add missing cases where vla sizes are not instrumented by UBSan [PR98608]

Message ID 01ed76d1286383f7a7e0a378be6c82bec12a2fd8.camel@tugraz.at
State Accepted
Headers
Series RFC [PATCH] c: Add missing cases where vla sizes are not instrumented by UBSan [PR98608] |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Martin Uecker Nov. 1, 2023, 9:39 a.m. UTC 
  Here is a patch that adds the missing cases for vla size instrumentation.
This now includes all cases where a type with size < 0 is created,
which is already UB and not just cases where a VLA is allocated.  But
a VLA can be allocated based on an typedef, which is also now
indirectly protected in this way.

I moved the instrumentation from the size itself as its own term into 
the expression that evaluate size expressions for side effects. This
avoids confusing other warning code that looks at the size expressions
(-Wvla-parameter).

There is one open question though:  How to to treat n == 0? 

Here I preliminary changed this to n > 0 (also for the existing case),
because when also detecting n == 0 this tools especially when
instrumenting all the types becomes basically useless because of 
the very common (and unproblematic) use of n == 0.  

But strictly speaking n == 0 is also UB and as pointed out in  PR98609
the error message is then not entirely accurate because it says
non-positive and not negative. I do not think it is confusing though
because it is still always correct.

One could consider splitting it up into vla-bound / vla-bound-strict,
but changing the error message would require further upstream changes
and dealing with this far exceeds the time I can afford contributing
to this this.

Another complication is that we ran out of bits for sanitizer flags in
unsigned int, so this would also require more changes.

Any advice?


I think it would be important to have complete UBSan coverage for all
size and bounds issues related to VM types and it would be nice to
get this in GCC 14. (I find this extremely useful in my projects).

Martin




    c: Add missing cases where vla sizes are not instrumented by UBSan [PR98608]
    
    Add vla-bound instrumentation for all VLAs including VLAs in parameters
    and fields, but allow zero-sized errors.
    
    Bootstrapped and regression tested on x86.
    
    PR c/98608
    
    gcc/c:
            * c-decl.cc (grokdeclarator): Instrument all VLAs.
    
    gcc/c-family:
            * c-ubsan.cc (ubsan_instrument_vla): Do not include zero length.
    
    gcc/testsuite:
            * gcc.dg/ubsan/pr89608.c: New test.
            * gcc.dg/ubsan/vla-1.c: New test.
            * gcc.dg/ubsan/vla-2.c: New test.
            * gcc.dg/ubsan/vla-3.c: New test.
            * c-c++-common/ubsan/vla-1.c: Adapt.
diff mbox series

Patch

diff --git a/gcc/c-family/c-ubsan.cc b/gcc/c-family/c-ubsan.cc
index b2c58c65d97..8983ede0166 100644
--- a/gcc/c-family/c-ubsan.cc
+++ b/gcc/c-family/c-ubsan.cc
@@ -313,7 +313,7 @@  ubsan_instrument_vla (location_t loc, tree size)
   tree type = TREE_TYPE (size);
   tree t, tt;
 
-  t = fold_build2 (LE_EXPR, boolean_type_node, size, build_int_cst (type, 0));
+  t = fold_build2 (LT_EXPR, boolean_type_node, size, build_int_cst (type, 0));
   if (flag_sanitize_trap & SANITIZE_VLA)
     tt = build_call_expr_loc (loc, builtin_decl_explicit (BUILT_IN_TRAP), 0);
   else
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index 7a145bed281..752a65d6729 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -7201,16 +7201,20 @@  grokdeclarator (const struct c_declarator *declarator,
 		       with known value.  */
 		    this_size_varies = size_varies = true;
 		    warn_variable_length_array (name, size);
-		    if (sanitize_flags_p (SANITIZE_VLA)
-			&& current_function_decl != NULL_TREE
-			&& decl_context == NORMAL)
+		    if (sanitize_flags_p (SANITIZE_VLA))
 		      {
 			/* Evaluate the array size only once.  */
 			size = save_expr (size);
 			size = c_fully_fold (size, false, NULL);
-		        size = fold_build2 (COMPOUND_EXPR, TREE_TYPE (size),
-					    ubsan_instrument_vla (loc, size),
-					    size);
+			tree instr = ubsan_instrument_vla (loc, size);
+			/* We have to build this in the right order, so
+			   instrumentation is done before the size can
+			   be used in other parameters.  */
+			if (*expr)
+			  *expr = build2 (COMPOUND_EXPR, TREE_TYPE (instr),
+					  *expr, instr);
+			else
+			  *expr = instr;
 		      }
 		  }
 
diff --git a/gcc/testsuite/c-c++-common/ubsan/vla-1.c b/gcc/testsuite/c-c++-common/ubsan/vla-1.c
index c97465edae1..cc441ffc80b 100644
--- a/gcc/testsuite/c-c++-common/ubsan/vla-1.c
+++ b/gcc/testsuite/c-c++-common/ubsan/vla-1.c
@@ -110,9 +110,7 @@  main (void)
 /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */
 /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -5\[^\n\r]*(\n|\r\n|\r)" } */
 /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -3\[^\n\r]*(\n|\r\n|\r)" } */
-/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value 0\[^\n\r]*(\n|\r\n|\r)" } */
 /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */
-/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value 0\[^\n\r]*(\n|\r\n|\r)" } */
 /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */
 /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */
 /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -6\[^\n\r]*(\n|\r\n|\r)" } */
diff --git a/gcc/testsuite/gcc.dg/ubsan/pr98608.c b/gcc/testsuite/gcc.dg/ubsan/pr98608.c
new file mode 100644
index 00000000000..9f8f40c9643
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ubsan/pr98608.c
@@ -0,0 +1,16 @@ 
+/* PR 98608 */
+/* { dg-do run } */
+/* { dg-options "-fsanitize=vla-bound" } */
+
+void f(int n, double (*x)[n])
+{
+	typeof(*x) y;	
+}
+
+int main()
+{
+	f(-1, 0);
+}
+
+/* { dg-output "variable length array bound evaluates to non-positive value -1" } */
+
diff --git a/gcc/testsuite/gcc.dg/ubsan/vla-1.c b/gcc/testsuite/gcc.dg/ubsan/vla-1.c
new file mode 100644
index 00000000000..6544698caf9
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ubsan/vla-1.c
@@ -0,0 +1,27 @@ 
+/* { dg-do run } */
+/* { dg-options "-Wstringop-overflow=0 -fsanitize=vla-bound -fsanitize-recover=vla-bound" } */
+
+void f1(int n, char buf[n]) { }
+/* { dg-output "variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */
+void f2(int n, float m[n][2]) { }
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */
+void f3(int n, int m, float x[n][m]) { }
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */
+void f4(int n) { typedef char buf[n]; }
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */
+void f5(int n) { struct { char buf[n]; } x; }
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */
+void f6(int n, struct { char buf[n]; } *p) { }	/* { dg-warning "anonymous" } */
+/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1" } */
+
+int main()
+{
+	f1(-1, 0);
+	f2(-1, 0);
+	f3(-1, 1, 0);
+	f4(-1);
+	f5(-1);
+	f6(-1, 0);
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/ubsan/vla-2.c b/gcc/testsuite/gcc.dg/ubsan/vla-2.c
new file mode 100644
index 00000000000..5937f860e89
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ubsan/vla-2.c
@@ -0,0 +1,21 @@ 
+/* { dg-do run } */
+/* { dg-options "-Wstringop-overflow=0 -fsanitize=vla-bound -fsanitize-recover=vla-bound" } */
+
+void f1(int n, char buf[n]) { }
+void f2(int n, float m[n][2]) { }
+void f3(int n, int m, float x[n][m]) { }
+void f4(int n) { typedef char buf[n]; }
+void f5(int n) { struct { char buf[n]; } x; }
+void f6(int n, struct { char buf[n]; } *p) { }	/* { dg-warning "anonymous" } */
+
+int main()
+{
+	f1(0, 0);
+	f2(0, 0);
+	f3(0, 1, 0);
+	f4(0);
+	f5(0);
+	f6(0, 0);
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/ubsan/vla-3.c b/gcc/testsuite/gcc.dg/ubsan/vla-3.c
new file mode 100644
index 00000000000..9f4b0555328
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ubsan/vla-3.c
@@ -0,0 +1,14 @@ 
+/* { dg-do run }
+ * { dg-options "-fsanitize=vla-bound" } */
+
+void foo(int n, char (*buf)[n], char p[n = 1])
+{
+}
+
+int main()
+{
+	foo(-1, 0, 0);
+}
+
+/* { dg-output "variable length array bound evaluates to non-positive value -1" } */
+