[C++ PATCH] Disable VLA initialization? (PR sanitizer/79993)

[C++ PATCH] Disable VLA initialization? (PR sanitizer/79993)

[Jakub Jelinek]

Hi!

GCC 4.8 and earlier didn't allow in C++ initialization of VLA and
C doesn't allow it in any GCC release.  This has changed when the
support for C++1y VLA has been added, then reverted, but apparently
only partially.

The question is, do we want to support VLA initialization, if yes,
with what exact semantics (in that case we need to fix up initialization
from STRING_CST which is broken right now).

The following patch is the variant that disables it (bootstrapped/regtested
on x86_64-linux and i686-linux).

Looking at what is emitted for initialization from non-STRING_CSTs,
it seems that we consider UB if the VLA is shorter than the initializer
array, and if it is longer, we somehow initialize the rest (dunno what
exact C++ initialization kind, there are some testcases
with ctors in the list below; for PODs zero initialization).

So, if we want to keep it and just fix STRING_CST initialization,
shall we emit memcpy from the array followed by whatever we emit right now
for the excess elements (for initialization from STRING_CST, can it ever
be anything but zero initialization?  If not, then memset would do the job).

2017-03-29  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/79993
	* decl.c (check_array_initializer): Reject VLA initialization.

	* g++.dg/init/array24.C: Adjust for VLA initialization being
	forbidden.
	* g++.dg/opt/pr78201.C: Likewise.
	* g++.dg/ubsan/vla-1.C: Likewise.
	* g++.dg/ext/constexpr-vla1.C: Likewise.
	* g++.dg/ext/constexpr-vla2.C: Likewise.
	* g++.dg/ext/constexpr-vla3.C: Likewise.
	* g++.dg/ext/constexpr-vla4.C: Likewise.
	* g++.dg/cpp1y/pr63996.C: Likewise.
	* g++.dg/cpp1y/vla-initlist1.C: Likewise.
	* g++.dg/cpp1y/vla2.C: Likewise.
	* g++.dg/cpp1y/vla10.C: Likewise.

--- gcc/cp/decl.c.jj	2017-03-21 07:57:00.000000000 +0100
+++ gcc/cp/decl.c	2017-03-29 09:45:20.741094070 +0200
@@ -6148,12 +6148,16 @@ check_array_initializer (tree decl, tree
 	error ("elements of array %q#T have incomplete type", type);
       return true;
     }
-  /* A compound literal can't have variable size.  */
-  if (init && !decl
+  /* It is not valid to initialize a VLA.  */
+  if (init
       && ((COMPLETE_TYPE_P (type) && !TREE_CONSTANT (TYPE_SIZE (type)))
 	  || !TREE_CONSTANT (TYPE_SIZE (element_type))))
     {
-      error ("variable-sized compound literal");
+      if (decl)
+	error_at (DECL_SOURCE_LOCATION (decl),
+		  "variable-sized object %qD may not be initialized", decl);
+      else
+	error ("variable-sized compound literal");
       return true;
     }
   return false;
--- gcc/testsuite/g++.dg/init/array24.C.jj	2016-04-14 21:17:01.000000000 +0200
+++ gcc/testsuite/g++.dg/init/array24.C	2017-03-29 15:20:03.593188437 +0200
@@ -3,5 +3,5 @@
 
 void foo(int i)
 {
-  int x[][i] = { 0 };
-}
+  int x[][i] = { 0 };	// { dg-error "variable-sized object .x. may not be initialized" }
+}			// { dg-error "storage size of .x. isn.t known" "" { target *-*-* } .-1 }
--- gcc/testsuite/g++.dg/opt/pr78201.C.jj	2016-11-17 18:08:39.000000000 +0100
+++ gcc/testsuite/g++.dg/opt/pr78201.C	2017-03-29 14:56:08.673071231 +0200
@@ -8,6 +8,6 @@ long e;
 void
 foo ()
 {
-  char a[e] = "";
+  char a[e] = "";	// { dg-error "variable-sized object 'a' may not be initialized" }
   c && c->d();
 }
--- gcc/testsuite/g++.dg/ubsan/vla-1.C.jj	2016-04-14 21:17:01.000000000 +0200
+++ gcc/testsuite/g++.dg/ubsan/vla-1.C	2017-03-29 14:59:31.477405607 +0200
@@ -1,9 +1,8 @@
-// { dg-do run }
+// { dg-do compile }
 // { dg-options "-Wno-vla -fsanitize=undefined" }
-// { dg-output "index 1 out of bounds" }
 
 void f(int i) {
-  int ar[i] = { 42, 24 };
+  int ar[i] = { 42, 24 };	// { dg-error "variable-sized object .ar. may not be initialized" }
 }
 
 int main()
--- gcc/testsuite/g++.dg/ext/constexpr-vla2.C.jj	2016-04-04 12:28:40.000000000 +0200
+++ gcc/testsuite/g++.dg/ext/constexpr-vla2.C	2017-03-29 14:52:55.648624577 +0200
@@ -4,7 +4,7 @@
 constexpr int
 fn_bad (int n)
 {
-  __extension__ int a [n] = { 0 };
+  __extension__ int a [n] = { 0 };		// { dg-error "variable-sized object .a. may not be initialized" }
   int z = a [0] + (n ? fn_bad (n - 1) : 0);
   return z;
 }
@@ -12,10 +12,10 @@ fn_bad (int n)
 constexpr int
 fn_ok (int n)
 {
-  __extension__ int a [n] = { 0 };
+  __extension__ int a [n] = { 0 };		// { dg-error "variable-sized object .a. may not be initialized" }
   int z = a [0] + (n > 1 ? fn_ok (n - 1) : 0);
   return z;
 }
 
-constexpr int i1 = fn_ok (3);
-constexpr int i2 = fn_bad (3); // { dg-error "array subscript" }
+constexpr int i1 = fn_ok (3);  // { dg-error "accessing uninitialized array element" }
+constexpr int i2 = fn_bad (3); // { dg-error "accessing uninitialized array element" }
--- gcc/testsuite/g++.dg/ext/constexpr-vla3.C.jj	2016-04-04 12:28:40.000000000 +0200
+++ gcc/testsuite/g++.dg/ext/constexpr-vla3.C	2017-03-29 14:53:52.174880426 +0200
@@ -4,11 +4,11 @@
 constexpr int
 foo (int n)
 {
-  __extension__ int a[n] = { 1, 2, 3, 4, 5, 6 };
+  __extension__ int a[n] = { 1, 2, 3, 4, 5, 6 };	// { dg-error "variable-sized object .a. may not be initialized" }
   int z = 0;
   for (int i = 0; i <= n; ++i)
     z += a[i];
   return z;
 }
 
-constexpr int n = foo (3); // { dg-error "array subscript" }
+constexpr int n = foo (3); // { dg-error "accessing uninitialized array element" }
--- gcc/testsuite/g++.dg/ext/constexpr-vla1.C.jj	2016-04-04 12:28:40.000000000 +0200
+++ gcc/testsuite/g++.dg/ext/constexpr-vla1.C	2017-03-29 14:21:13.836697832 +0200
@@ -4,7 +4,7 @@
 constexpr int
 fn_ok (int n)
 {
-    __extension__ int a[n] = { };
+    __extension__ int a[n] = { };	// { dg-error "variable-sized object .a. may not be initialized" }
     int z = 0;
 
     for (unsigned i = 0; i < sizeof (a) / sizeof (int); ++i)
@@ -17,7 +17,7 @@ fn_ok (int n)
 constexpr int
 fn_not_ok (int n)
 {
-    __extension__ int a[n] = { };
+    __extension__ int a[n] = { };	// { dg-error "variable-sized object .a. may not be initialized" }
     int z = 0;
 
     for (unsigned i = 0; i < sizeof (a); ++i)
@@ -26,5 +26,5 @@ fn_not_ok (int n)
     return z;
 }
 
-constexpr int n1 = fn_ok (3);
-constexpr int n2 = fn_not_ok (3); // { dg-error "array subscript" }
+constexpr int n1 = fn_ok (3);	  // { dg-error "accessing uninitialized array element" }
+constexpr int n2 = fn_not_ok (3); // { dg-error "accessing uninitialized array element" }
--- gcc/testsuite/g++.dg/ext/constexpr-vla4.C.jj	2016-04-05 18:57:01.000000000 +0200
+++ gcc/testsuite/g++.dg/ext/constexpr-vla4.C	2017-03-29 14:54:54.589053166 +0200
@@ -4,7 +4,7 @@
 constexpr int
 foo (int n, bool p)
 {
-  __extension__ int a [n] = { 0 };
+  __extension__ int a [n] = { 0 };	// { dg-error "variable-sized object 'a' may not be initialized" }
   if (n == 3)
     foo (n - 2, false);
   if (n == 3)
@@ -15,3 +15,4 @@ foo (int n, bool p)
 }
 
 constexpr int i2 = foo (3, false); // { dg-bogus "array subscript out of bound" }
+				   // { dg-error "accessing uninitialized array element" "" { target *-*-* } .-1 }
--- gcc/testsuite/g++.dg/cpp1y/vla2.C.jj	2014-12-12 08:59:22.000000000 +0100
+++ gcc/testsuite/g++.dg/cpp1y/vla2.C	2017-03-29 14:18:47.996628357 +0200
@@ -1,10 +1,10 @@
 // N3639 allows initialization and capture of VLAs
-// { dg-do run { target c++11 } }
+// { dg-do compile { target c++11 } }
 // { dg-options "-Wno-vla" }
 
 void f(int n)
 {
-  int ar[n] = { 42 };
+  int ar[n] = { 42 };			// { dg-error "variable-sized object 'ar' may not be initialized" }
   auto l = [&] { return ar[0]; };
   if (l() != 42) __builtin_abort ();
 }
--- gcc/testsuite/g++.dg/cpp1y/vla-initlist1.C.jj	2017-02-09 12:59:40.000000000 +0100
+++ gcc/testsuite/g++.dg/cpp1y/vla-initlist1.C	2017-03-29 15:16:57.151656875 +0200
@@ -1,5 +1,4 @@
-// { dg-do run { target c++11 } }
-// { dg-skip-if "power overwrites two slots of array i" { "power*-*-*" } }
+// { dg-do compile { target c++11 } }
 // { dg-options "-Wno-vla" }
 
 #include <initializer_list>
@@ -15,9 +14,9 @@ struct A
 int x = 4;
 int main(int argc, char **argv)
 {
-  { int i[x] = { 42, 42, 42, 42 }; }
+  { int i[x] = { 42, 42, 42, 42 }; }	// { dg-error "variable-sized object .i. may not be initialized" }
   {
-    A a[x] = { argc };
+    A a[x] = { argc };			// { dg-error "variable-sized object .a. may not be initialized" }
     if (a[1].i != 42)
       __builtin_abort ();
   }
--- gcc/testsuite/g++.dg/cpp1y/pr63996.C.jj	2015-01-14 09:55:17.000000000 +0100
+++ gcc/testsuite/g++.dg/cpp1y/pr63996.C	2017-03-29 14:11:05.619748966 +0200
@@ -4,7 +4,7 @@ constexpr int
 foo (int i)
 {
   int a[i] = { }; // { dg-error "forbids variable length" }
-}
+}		  // { dg-error "variable-sized object .a. may not be initialized" "" { target *-*-* } .-1 }
 
 constexpr int j = foo (1); // { dg-error "flows off the end" }
 
--- gcc/testsuite/g++.dg/cpp1y/vla10.C.jj	2014-12-12 08:59:22.000000000 +0100
+++ gcc/testsuite/g++.dg/cpp1y/vla10.C	2017-03-29 14:17:39.552534370 +0200
@@ -1,5 +1,5 @@
 // PR c++/57402
-// { dg-do run }
+// { dg-do compile }
 // { dg-options "" }
 
 int i = 2;
@@ -11,13 +11,13 @@ int main()
     a[1] = 0xbeef;
   }
   {
-    int a[i] = { 1 };
+    int a[i] = { 1 };		// { dg-error "variable-sized object .a. may not be initialized" }
     if (a[1] != 0)
       __builtin_abort ();
     a[1] = 0xbeef;
   }
   {
-    int a[i] = { };
+    int a[i] = { };		// { dg-error "variable-sized object .a. may not be initialized" }
     if (a[1] != 0)
       __builtin_abort ();
     a[1] = 0xbeef;

	Jakub

Re: [C++ PATCH] Disable VLA initialization? (PR sanitizer/79993)

[Martin Sebor]

On 03/29/2017 01:31 PM, Jakub Jelinek wrote:
> Hi!
>
> GCC 4.8 and earlier didn't allow in C++ initialization of VLA and
> C doesn't allow it in any GCC release.  This has changed when the
> support for C++1y VLA has been added, then reverted, but apparently
> only partially.
>
> The question is, do we want to support VLA initialization, if yes,
> with what exact semantics (in that case we need to fix up initialization
> from STRING_CST which is broken right now).
>
> The following patch is the variant that disables it (bootstrapped/regtested
> on x86_64-linux and i686-linux).
>
> Looking at what is emitted for initialization from non-STRING_CSTs,
> it seems that we consider UB if the VLA is shorter than the initializer
> array, and if it is longer, we somehow initialize the rest (dunno what
> exact C++ initialization kind, there are some testcases
> with ctors in the list below; for PODs zero initialization).
>
> So, if we want to keep it and just fix STRING_CST initialization,
> shall we emit memcpy from the array followed by whatever we emit right now
> for the excess elements (for initialization from STRING_CST, can it ever
> be anything but zero initialization?  If not, then memset would do the job).

I already chimed in on the bug report but just for the record,
my view is that it would be preferable to continue to allow VLAs
to be initialized as long as the excess initialization isn't
allowed to trash the stack as it is now (bug 69517).  The patch
for that was committed into GCC 6 but had to be reverted due to
interfering with Java's own implementation of exception).  With
Java removed I'd like to resubmit the patch in GCC 8.

I don't think rejecting all VLA initialization just to avoid
an Asan ICE with string literals is a good way to solve the
problem.  For one, it will break working programs that rely on
what is currently accepted by GCC and for which it emits valid
code.  Longer term, users will likely come up with other ways
to initialize their VLAs that may be even harder (or slower)
for GCC to detect bugs in that the plain initialization.

Martin

Re: [C++ PATCH] Disable VLA initialization? (PR sanitizer/79993)

[Jakub Jelinek]

On Thu, Mar 30, 2017 at 10:21:09AM -0600, Martin Sebor wrote:
> I don't think rejecting all VLA initialization just to avoid
> an Asan ICE with string literals is a good way to solve the
> problem.  For one, it will break working programs that rely on

The Asan ICE can be easily worked around, the reason I don't want to do that
is that this is the second time this broke something; the FE should
not pass a VLA static constant (STRING_CST in this case) to the middle-end,
VLA objects can be by definition only automatic variables or something
on the heap, never in the data/rodata sections.
Plus, while for VLA initialized from arrays etc. we copy the initializer
and zero initialize the rest, for VLA initialization by STRING_CST we
just copy over the STRING_CST and the rest is uninitialized.

	Jakub

Re: [C++ PATCH] Disable VLA initialization? (PR sanitizer/79993)

[Martin Sebor]

On 03/30/2017 02:24 PM, Jakub Jelinek wrote:
> On Thu, Mar 30, 2017 at 10:21:09AM -0600, Martin Sebor wrote:
>> I don't think rejecting all VLA initialization just to avoid
>> an Asan ICE with string literals is a good way to solve the
>> problem.  For one, it will break working programs that rely on
>
> The Asan ICE can be easily worked around, the reason I don't want to do that
> is that this is the second time this broke something; the FE should
> not pass a VLA static constant (STRING_CST in this case) to the middle-end,
> VLA objects can be by definition only automatic variables or something
> on the heap, never in the data/rodata sections.
> Plus, while for VLA initialized from arrays etc. we copy the initializer
> and zero initialize the rest, for VLA initialization by STRING_CST we
> just copy over the STRING_CST and the rest is uninitialized.

I agree that's a bug, one that should be fixed, but ion my view
it's not serious enough to justify disabling the feature altogether.

If every feature with a bug in it or that happens to trigger an ICE
with some combination of command line options had to be disabled
there wouldn't be much left.

As I said, a patch that fixes this exists.  I ran out of time to
resubmit it for GCC 7 but I plan to do it for GCC 8.  It be quite
unfriendly to users to have GCC 7 reject working code that's
accepted by GCC 6 only to have GCC 8 accept it again.

I know you have concerns about the runtime costs of the patch
and I'm willing to listen and work with you to minimize the
performance impact.

Martin