diff mbox series

avoid assuming arrays have nonzero size (PR 88956)

Message ID 8d56ad7d-88d9-b5b9-35da-4d7fadf2ec00@gmail.com
State New
Headers show
Series avoid assuming arrays have nonzero size (PR 88956) | expand

Commit Message

Martin Sebor Jan. 23, 2019, 1:18 a.m. UTC 
The enhancement to treat char initializer-lists as STRING_CSTs
committed earlier last year introduced an assumption that array
elements have a non-zero size.  As it turns out, the zero-length
array extension that makes it possible to define even multi-
dimensional zero-length array objects breaks that assumption when
it's passed as an argument to a function like memcpy. The attached
patch removes that assumption.

Tested on x86_64-linux.

Martin

PS In GCC 10, unless there is an important use case that escapes
me, I think GCC should warn for zero-length non-member array
objects, or perhaps even for internal struct members (those followed
by another member).  Not to avoid these sorts of bugs but because
they seem too dangerous to use safely.

Comments

Joseph Myers Jan. 23, 2019, 1:33 a.m. UTC | #1 
On Tue, 22 Jan 2019, Martin Sebor wrote:

> PS In GCC 10, unless there is an important use case that escapes
> me, I think GCC should warn for zero-length non-member array
> objects, or perhaps even for internal struct members (those followed
> by another member).  Not to avoid these sorts of bugs but because
> they seem too dangerous to use safely.

I think one expected use case of zero-size objects is for e.g. a structure 
that is zero-size in some configurations of the code and non-zero-size on 
other configurations (but where it's most convenient to have the 
variables, struct members, etc. exist unconditionally).  It's quite 
possible that arrays of such objects may be constructed in such code.
Martin Sebor Jan. 29, 2019, 4:03 a.m. UTC | #2 
Ping: https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01340.html

This is a straightforward fix for an ICE.  I will commit it tomorrow
unless there are suggestions for other changes.

On 1/22/19 6:18 PM, Martin Sebor wrote:
> The enhancement to treat char initializer-lists as STRING_CSTs
> committed earlier last year introduced an assumption that array
> elements have a non-zero size.  As it turns out, the zero-length
> array extension that makes it possible to define even multi-
> dimensional zero-length array objects breaks that assumption when
> it's passed as an argument to a function like memcpy. The attached
> patch removes that assumption.
> 
> Tested on x86_64-linux.
> 
> Martin
> 
> PS In GCC 10, unless there is an important use case that escapes
> me, I think GCC should warn for zero-length non-member array
> objects, or perhaps even for internal struct members (those followed
> by another member).  Not to avoid these sorts of bugs but because
> they seem too dangerous to use safely.
Richard Sandiford Jan. 29, 2019, 12:44 p.m. UTC | #3 
Martin Sebor <msebor@gmail.com> writes:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01340.html
>
> This is a straightforward fix for an ICE.  I will commit it tomorrow
> unless there are suggestions for other changes.

That's not how it works.  Please wait for the patch to be approved
by someone.

Thanks,
Richard
Martin Sebor Jan. 29, 2019, 3:51 p.m. UTC | #4 
On 1/29/19 5:44 AM, Richard Sandiford wrote:
> Martin Sebor <msebor@gmail.com> writes:
>> Ping: https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01340.html
>>
>> This is a straightforward fix for an ICE.  I will commit it tomorrow
>> unless there are suggestions for other changes.
> 
> That's not how it works.  Please wait for the patch to be approved
> by someone.

The rules are explicit that "Obvious fixes can be committed without
prior approval."  I consider moving an equality test three lines up
an obvious fix.

Martin
Jakub Jelinek Jan. 29, 2019, 6:23 p.m. UTC | #5 
On Tue, Jan 22, 2019 at 06:18:28PM -0700, Martin Sebor wrote:
> PS In GCC 10, unless there is an important use case that escapes
> me, I think GCC should warn for zero-length non-member array
> objects, or perhaps even for internal struct members (those followed
> by another member).  Not to avoid these sorts of bugs but because
> they seem too dangerous to use safely.

No, we shouldn't warn.

> --- gcc/gimple-fold.c	(revision 268086)
> +++ gcc/gimple-fold.c	(working copy)
> @@ -6715,12 +6715,14 @@ fold_array_ctor_reference (tree type, tree ctor,
>    elt_size = wi::to_offset (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (ctor))));
>  
>    /* When TYPE is non-null, verify that it specifies a constant-sized
> -     accessed not larger than size of array element.  */
> -  if (type
> -      && (!TYPE_SIZE_UNIT (type)
> -	  || TREE_CODE (TYPE_SIZE_UNIT (type)) != INTEGER_CST
> -	  || elt_size < wi::to_offset (TYPE_SIZE_UNIT (type))
> -	  || elt_size == 0))
> +     accessed not larger than size of array element.  Avoid using zero
> +     ELT_SIZE, the result of an empty initializer for a zero-length
> +     array.  */

The comment is misleading, there are many reasons why you could have
zero elt_size, C struct S {}, or int x[10][0], etc.  Just don't change
anything in the comment at all, or say 
Punt if element size is zero to avoid division by zero.
or something similar.

> +  if (elt_size == 0
> +      || (type
> +	  && (!TYPE_SIZE_UNIT (type)
> +	      || TREE_CODE (TYPE_SIZE_UNIT (type)) != INTEGER_CST
> +	      || elt_size < wi::to_offset (TYPE_SIZE_UNIT (type)))))
>      return NULL_TREE;

Otherwise LGTM.

If you could fix also the two comments a few lines above this:
  /* Static constructors for variably sized objects makes no sense.  */
to
  /* Static constructors for variably sized objects make no sense.  */
it would be appreciated.

	Jakub
Martin Sebor Jan. 30, 2019, 3:06 a.m. UTC | #6 
On 1/29/19 11:23 AM, Jakub Jelinek wrote:
>> --- gcc/gimple-fold.c	(revision 268086)
>> +++ gcc/gimple-fold.c	(working copy)
>> @@ -6715,12 +6715,14 @@ fold_array_ctor_reference (tree type, tree ctor,
>>     elt_size = wi::to_offset (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (ctor))));
>>   
>>     /* When TYPE is non-null, verify that it specifies a constant-sized
>> -     accessed not larger than size of array element.  */
>> -  if (type
>> -      && (!TYPE_SIZE_UNIT (type)
>> -	  || TREE_CODE (TYPE_SIZE_UNIT (type)) != INTEGER_CST
>> -	  || elt_size < wi::to_offset (TYPE_SIZE_UNIT (type))
>> -	  || elt_size == 0))
>> +     accessed not larger than size of array element.  Avoid using zero
>> +     ELT_SIZE, the result of an empty initializer for a zero-length
>> +     array.  */
> 
> The comment is misleading, there are many reasons why you could have
> zero elt_size, C struct S {}, or int x[10][0], etc.  Just don't change
> anything in the comment at all, or say
> Punt if element size is zero to avoid division by zero.
> or something similar.

Thanks.  The goal of the comment is to give an example of the unusual
case when the size of an array element may be zero, not suggest there
is just one such case, or enumerate all all of them.  I've adjusted
the text and added tests for the empty zero struct.

> 
>> +  if (elt_size == 0
>> +      || (type
>> +	  && (!TYPE_SIZE_UNIT (type)
>> +	      || TREE_CODE (TYPE_SIZE_UNIT (type)) != INTEGER_CST
>> +	      || elt_size < wi::to_offset (TYPE_SIZE_UNIT (type)))))
>>       return NULL_TREE;
> 
> Otherwise LGTM.
> 
> If you could fix also the two comments a few lines above this:
>    /* Static constructors for variably sized objects makes no sense.  */
> to
>    /* Static constructors for variably sized objects make no sense.  */
> it would be appreciated.

Sure, I don't mind doing that.

Committed in r268378.

Martin
diff mbox series

Patch

PR middle-end/88956 - ICE: Floating point exception on a memcpy from an zero-length constant array

gcc/ChangeLog:

	PR c/88956
	* gimple-fold.c (fold_array_ctor_reference): Avoid zero-length arrays.

gcc/testsuite/ChangeLog:

	PR c/88956
	* gcc.dg/Warray-bounds-39.c: New test.

Index: gcc/gimple-fold.c
===================================================================
--- gcc/gimple-fold.c	(revision 268086)
+++ gcc/gimple-fold.c	(working copy)
@@ -6715,12 +6715,14 @@  fold_array_ctor_reference (tree type, tree ctor,
   elt_size = wi::to_offset (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (ctor))));
 
   /* When TYPE is non-null, verify that it specifies a constant-sized
-     accessed not larger than size of array element.  */
-  if (type
-      && (!TYPE_SIZE_UNIT (type)
-	  || TREE_CODE (TYPE_SIZE_UNIT (type)) != INTEGER_CST
-	  || elt_size < wi::to_offset (TYPE_SIZE_UNIT (type))
-	  || elt_size == 0))
+     accessed not larger than size of array element.  Avoid using zero
+     ELT_SIZE, the result of an empty initializer for a zero-length
+     array.  */
+  if (elt_size == 0
+      || (type
+	  && (!TYPE_SIZE_UNIT (type)
+	      || TREE_CODE (TYPE_SIZE_UNIT (type)) != INTEGER_CST
+	      || elt_size < wi::to_offset (TYPE_SIZE_UNIT (type)))))
     return NULL_TREE;
 
   /* Compute the array index we look for.  */
Index: gcc/testsuite/gcc.dg/Warray-bounds-39.c
===================================================================
--- gcc/testsuite/gcc.dg/Warray-bounds-39.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/Warray-bounds-39.c	(working copy)
@@ -0,0 +1,115 @@ 
+/* PR middle-end/88956 - ICE: Floating point exception on a memcpy from
+   an zero-length constant array
+   Verify both that memory and string calls with a zero-length array
+   don't cause an ICE, and also that they emit warnings.
+   { dg-do compile }
+   { dg-options "-O2 -Wall" }  */
+
+typedef __SIZE_TYPE__ size_t;
+
+extern void* memcpy (void*, const void*, size_t);
+extern void* memmove (void*, const void*, size_t);
+extern char* strcpy (char*, const char*);
+extern char* strncpy (char*, const char*, size_t);
+
+const char s0[0] = { };
+const char s0_0[0][0] = { };
+const char s0_1[0][1] = { };
+const char s1_0[1][0] = { };
+
+char d[4];
+
+void* test_memcpy_s0_1 (void *d)
+{
+  return memcpy (d, s0, 1);       /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+void* test_memcpy_s0_2 (void *d)
+{
+  return memcpy (d, s0, 2);       /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+void* test_memcpy_s0_0_1 (void *d)
+{
+  return memcpy (d, s0_0, 1);     /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+void* test_memcpy_s0_0_2 (void *d)
+{
+  return memcpy (d, s0_0, 2);     /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+
+void* test_memcpy_s0_1_1 (void *d)
+{
+  return memcpy (d, s0_1, 1);     /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+void* test_memcpy_s0_1_2 (void *d)
+{
+  return memcpy (d, s0_1, 2);     /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+
+void* test_memcpy_s1_0_1 (void *d)
+{
+  return memcpy (d, s1_0, 1);     /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+void* test_memcpy_s1_0_2 (void *d)
+{
+  return memcpy (d, s1_0, 2);     /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+
+void* test_memmove_s0_1 (void *d)
+{
+  return memmove (d, s0, 1);      /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+void* test_memmove_s0_2 (void *d)
+{
+  return memmove (d, s0, 2);      /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+void* test_memmove_s0_0_1 (void *d)
+{
+  return memmove (d, s0_0, 1);    /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+void* test_memmove_s0_0_2 (void *d)
+{
+  return memmove (d, s0_0, 2);    /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+
+char* test_strcpy_s0 (char *d)
+{
+  return strcpy (d, s0);          /* { dg-warning "\\\[-Warray-bounds" "pr88991" { xfail *-*-* } } */
+}
+
+char* test_strcpy_s0_0 (char *d)
+{
+  return strcpy (d, s0_0[0]);     /* { dg-warning "\\\[-Warray-bounds" "pr88991" { xfail *-*-* } } */
+}
+
+
+char* test_strncpy_s0_1 (char *d)
+{
+  return strncpy (d, s0, 1);    /* { dg-warning "\\\[-Warray-bounds" "pr88991" { xfail *-*-* } } */
+}
+
+char* test_strncpy_s0_2 (char *d)
+{
+  return strncpy (d, s0, 2);    /* { dg-warning "\\\[-Warray-bounds" "pr88991" { xfail *-*-* } } */
+}
+
+char* test_strncpy_s0_0_1 (char *d)
+{
+  return strncpy (d, s0_0[0], 1); /* { dg-warning "\\\[-Warray-bounds" "pr88991" { xfail *-*-* } } */
+}
+
+char* test_strncpy_s0_0_2 (char *d)
+{
+  return strncpy (d, s0_0[0], 2); /* { dg-warning "\\\[-Warray-bounds" "pr88991" { xfail *-*-* } } */
+}