[PATCH] Improve checks in c_strlen (PR 87053)

[PATCH] Improve checks in c_strlen (PR 87053)

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 383 bytes --]

Hi!


This patch adds some more checks to c_getstr to fix PR middle-end/87053
wrong code bug.

Unfortunately this patch alone is not sufficient to fix the problem,
but also the patch for PR 86714 that hardens c_getstr is necessary
to prevent the wrong folding.


Bootstrapped and reg-tested on top of my PR 86711/86714 patch.
Is it OK for trunk?


Thanks
Bernd.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-pr87053.diff --]
[-- Type: text/x-patch; name="patch-pr87053.diff", Size: 2227 bytes --]

gcc:
2018-08-22  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR middle-end/87053
	* builtins.c (c_strlen): Improve range checks.

testsuite:
2018-08-22  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR middle-end/87053
	* gcc.c-torture/execute/pr87053.c: New test.

diff -Npur gcc/builtins.c gcc/builtins.c
--- gcc/builtins.c	2018-08-17 05:02:16.000000000 +0200
+++ gcc/builtins.c	2018-08-22 08:51:21.287960030 +0200
@@ -576,7 +576,7 @@ string_length (const void *ptr, unsigned
 tree
 c_strlen (tree src, int only_value, unsigned eltsize)
 {
-  gcc_assert (eltsize == 1 || eltsize == 2 || eltsize == 4);
+  gcc_checking_assert (eltsize == 1 || eltsize == 2 || eltsize == 4);
   STRIP_NOPS (src);
   if (TREE_CODE (src) == COND_EXPR
       && (only_value || !TREE_SIDE_EFFECTS (TREE_OPERAND (src, 0))))
@@ -665,10 +665,10 @@ c_strlen (tree src, int only_value, unsi
      a null character if we can represent it as a single HOST_WIDE_INT.  */
   if (byteoff == 0)
     eltoff = 0;
-  else if (! tree_fits_shwi_p (byteoff))
+  else if (! tree_fits_uhwi_p (byteoff) || tree_to_uhwi (byteoff) % eltsize)
     eltoff = -1;
   else
-    eltoff = tree_to_shwi (byteoff) / eltsize;
+    eltoff = tree_to_uhwi (byteoff) / eltsize;
 
   /* If the offset is known to be out of bounds, warn, and call strlen at
      runtime.  */
@@ -700,6 +700,10 @@ c_strlen (tree src, int only_value, unsi
   unsigned len = string_length (ptr + eltoff * eltsize, eltsize,
 				strelts - eltoff);
 
+  /* Don't know what to return if there was no zero termination.  */
+  if (len > maxelts - eltoff)
+    return NULL_TREE;
+
   return ssize_int (len);
 }
 
diff -Npur gcc/testsuite/gcc.c-torture/execute/pr87053.c gcc/testsuite/gcc.c-torture/execute/pr87053.c
--- gcc/testsuite/gcc.c-torture/execute/pr87053.c	1970-01-01 01:00:00.000000000 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr87053.c	2018-08-22 12:54:00.801019240 +0200
@@ -0,0 +1,17 @@
+/* PR middle-end/87053 */
+
+const union
+{ struct {
+    char x[4];
+    char y[4];
+  };
+  struct {
+    char z[8];
+  };
+} u = {{"1234", "567"}};
+
+int main ()
+{
+  if (__builtin_strlen (u.z) != 7)
+    __builtin_abort ();
+}

Re: [PATCH] Improve checks in c_strlen (PR 87053)

[Martin Sebor]

On 08/22/2018 08:41 AM, Bernd Edlinger wrote:
> Hi!
>
>
> This patch adds some more checks to c_getstr to fix PR middle-end/87053
> wrong code bug.
>
> Unfortunately this patch alone is not sufficient to fix the problem,
> but also the patch for PR 86714 that hardens c_getstr is necessary
> to prevent the wrong folding.
>
>
> Bootstrapped and reg-tested on top of my PR 86711/86714 patch.
> Is it OK for trunk?

This case is also the subject of the patch I submitted back in
July for 86711/86714 and 86552.  With it, GCC avoid folding
the strlen call early and warns for the missing nul:

warning: ‘__builtin_strlen’ argument missing terminating nul 
[-Wstringop-overflow=]
    if (__builtin_strlen (u.z) != 7)
        ^~~~~~~~~~~~~~~~

The patch doesn't doesn't prevent all such strings from being
folded and it eventually lets fold_builtin_strlen() do its thing:

	  /* To avoid warning multiple times about unterminated
	     arrays only warn if its length has been determined
	     and is being folded to a constant.  */
	  if (nonstr)
	    warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr);

	  return fold_convert_loc (loc, type, len);

Handling this case is a matter of avoiding the folding here as
well and moving the warning later.

Since my patch is still in the review queue and does much more
than just prevent folding of non-nul terminated arrays it should
be reviewed first.

Martin

Re: [PATCH] Improve checks in c_strlen (PR 87053)

[Bernd Edlinger]

On 08/22/18 18:28, Martin Sebor wrote:
> On 08/22/2018 08:41 AM, Bernd Edlinger wrote:
>> Hi!
>>
>>
>> This patch adds some more checks to c_getstr to fix PR middle-end/87053
>> wrong code bug.
>>
>> Unfortunately this patch alone is not sufficient to fix the problem,
>> but also the patch for PR 86714 that hardens c_getstr is necessary
>> to prevent the wrong folding.
>>
>>
>> Bootstrapped and reg-tested on top of my PR 86711/86714 patch.
>> Is it OK for trunk?
> 
> This case is also the subject of the patch I submitted back in
> July for 86711/86714 and 86552.  With it, GCC avoid folding
> the strlen call early and warns for the missing nul:
> 
> warning: ‘__builtin_strlen’ argument missing terminating nul [-Wstringop-overflow=]
>     if (__builtin_strlen (u.z) != 7)
>         ^~~~~~~~~~~~~~~~
> 
> The patch doesn't doesn't prevent all such strings from being
> folded and it eventually lets fold_builtin_strlen() do its thing:
> 
>        /* To avoid warning multiple times about unterminated
>           arrays only warn if its length has been determined
>           and is being folded to a constant.  */
>        if (nonstr)
>          warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr);
> 
>        return fold_convert_loc (loc, type, len);
> 
> Handling this case is a matter of avoiding the folding here as
> well and moving the warning later.
> 
> Since my patch is still in the review queue and does much more
> than just prevent folding of non-nul terminated arrays it should
> be reviewed first.
> 

Hmmm, now you made me curious.

So I tried to install your patch (I did this on r263508
since it does not apply to trunk, one thing I noted is
that part 4 and part 3 seem to create gcc/testsuite/gcc.dg/warn-strcpy-no-nul.c
I did not check if they are identical or not).

So I tried the test case from this PR on the compiler built with your patch:

$ cat cat pr87053.c
/* PR middle-end/87053 */

const union
{ struct {
     char x[4];
     char y[4];
   };
   struct {
     char z[8];
   };
} u = {{"1234", "567"}};

int main ()
{
   if (__builtin_strlen (u.z) != 7)
     __builtin_abort ();
}
$ gcc -S pr87053.c
pr87053.c: In function 'main':
pr87053.c:15:7: warning: '__builtin_strlen' argument missing terminating nul [-Wstringop-overflow=]
15 |   if (__builtin_strlen (u.z) != 7)
    |       ^~~~~~~~~~~~~~~~
pr87053.c:11:3: note: referenced argument declared here
11 | } u = {{"1234", "567"}};
    |   ^
$ cat pr87053.s
	.file	"pr87053.c"
	.text
	.globl	u
	.section	.rodata
	.align 8
	.type	u, @object
	.size	u, 8
u:
	.ascii	"1234"
	.string	"567"
	.text
	.globl	main
	.type	main, @function
main:
.LFB0:
	.cfi_startproc
	pushq	%rbp
	.cfi_def_cfa_offset 16
	.cfi_offset 6, -16
	movq	%rsp, %rbp
	.cfi_def_cfa_register 6
	call	abort
	.cfi_endproc
.LFE0:
	.size	main, .-main
	.ident	"GCC: (GNU) 9.0.0 20180813 (experimental)"
	.section	.note.GNU-stack,"",@progbits


So we get a warning, and still wrong code.

That is the reason why I think this patch of yours adds
confusion by trying to fix everything in one step.

And I would like you to think of ways how to solve
a problem step by step.

And at this time, sorry, we should restore correctness issues.
And fix wrong-code issues.
If possible without breaking existing warnings, yes.
But no new warnings, sorry again.


Bernd.

Re: [PATCH] Improve checks in c_strlen (PR 87053)

[Jeff Law]

On 08/23/2018 03:27 AM, Bernd Edlinger wrote:
> On 08/22/18 18:28, Martin Sebor wrote:
>> On 08/22/2018 08:41 AM, Bernd Edlinger wrote:
>>> Hi!
>>>
>>>
>>> This patch adds some more checks to c_getstr to fix PR middle-end/87053
>>> wrong code bug.
>>>
>>> Unfortunately this patch alone is not sufficient to fix the problem,
>>> but also the patch for PR 86714 that hardens c_getstr is necessary
>>> to prevent the wrong folding.
>>>
>>>
>>> Bootstrapped and reg-tested on top of my PR 86711/86714 patch.
>>> Is it OK for trunk?
>>
>> This case is also the subject of the patch I submitted back in
>> July for 86711/86714 and 86552.  With it, GCC avoid folding
>> the strlen call early and warns for the missing nul:
>>
>> warning: ‘__builtin_strlen’ argument missing terminating nul [-Wstringop-overflow=]
>>     if (__builtin_strlen (u.z) != 7)
>>         ^~~~~~~~~~~~~~~~
>>
>> The patch doesn't doesn't prevent all such strings from being
>> folded and it eventually lets fold_builtin_strlen() do its thing:
>>
>>        /* To avoid warning multiple times about unterminated
>>           arrays only warn if its length has been determined
>>           and is being folded to a constant.  */
>>        if (nonstr)
>>          warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr);
>>
>>        return fold_convert_loc (loc, type, len);
>>
>> Handling this case is a matter of avoiding the folding here as
>> well and moving the warning later.
>>
>> Since my patch is still in the review queue and does much more
>> than just prevent folding of non-nul terminated arrays it should
>> be reviewed first.
>>
> 
> Hmmm, now you made me curious.
> 
> So I tried to install your patch (I did this on r263508
> since it does not apply to trunk, one thing I noted is
> that part 4 and part 3 seem to create gcc/testsuite/gcc.dg/warn-strcpy-no-nul.c
> I did not check if they are identical or not).
> 
> So I tried the test case from this PR on the compiler built with your patch:
> 
> $ cat cat pr87053.c
> /* PR middle-end/87053 */
> 
> const union
> { struct {
>      char x[4];
>      char y[4];
>    };
>    struct {
>      char z[8];
>    };
> } u = {{"1234", "567"}};
> 
> int main ()
> {
>    if (__builtin_strlen (u.z) != 7)
>      __builtin_abort ();
> }
> $ gcc -S pr87053.c
> pr87053.c: In function 'main':
> pr87053.c:15:7: warning: '__builtin_strlen' argument missing terminating nul [-Wstringop-overflow=]
> 15 |   if (__builtin_strlen (u.z) != 7)
>     |       ^~~~~~~~~~~~~~~~
> pr87053.c:11:3: note: referenced argument declared here
> 11 | } u = {{"1234", "567"}};
>     |   ^
> $ cat pr87053.s
> 	.file	"pr87053.c"
> 	.text
> 	.globl	u
> 	.section	.rodata
> 	.align 8
> 	.type	u, @object
> 	.size	u, 8
> u:
> 	.ascii	"1234"
> 	.string	"567"
> 	.text
> 	.globl	main
> 	.type	main, @function
> main:
> .LFB0:
> 	.cfi_startproc
> 	pushq	%rbp
> 	.cfi_def_cfa_offset 16
> 	.cfi_offset 6, -16
> 	movq	%rsp, %rbp
> 	.cfi_def_cfa_register 6
> 	call	abort
> 	.cfi_endproc
> .LFE0:
> 	.size	main, .-main
> 	.ident	"GCC: (GNU) 9.0.0 20180813 (experimental)"
> 	.section	.note.GNU-stack,"",@progbits
> 
> 
> So we get a warning, and still wrong code.
> 
> That is the reason why I think this patch of yours adds
> confusion by trying to fix everything in one step.
> 
> And I would like you to think of ways how to solve
> a problem step by step.
> 
> And at this time, sorry, we should restore correctness issues.
> And fix wrong-code issues.
> If possible without breaking existing warnings, yes.
> But no new warnings, sorry again.
Just a note, Martin's most fix for 86711/86714 fixes codegen issues
without breaking existing warnings or adding new warnings.  The new
warnings were broken out into follow-up patches.

jeff
> Bernd.
> 

Re: [PATCH] Improve checks in c_strlen (PR 87053)

[Bernd Edlinger]

On 08/24/18 07:58, Jeff Law wrote:
> On 08/23/2018 03:27 AM, Bernd Edlinger wrote:
>> On 08/22/18 18:28, Martin Sebor wrote:
>>> On 08/22/2018 08:41 AM, Bernd Edlinger wrote:
>>>> Hi!
>>>>
>>>>
>>>> This patch adds some more checks to c_getstr to fix PR middle-end/87053
>>>> wrong code bug.
>>>>
>>>> Unfortunately this patch alone is not sufficient to fix the problem,
>>>> but also the patch for PR 86714 that hardens c_getstr is necessary
>>>> to prevent the wrong folding.
>>>>
>>>>
>>>> Bootstrapped and reg-tested on top of my PR 86711/86714 patch.
>>>> Is it OK for trunk?
>>>
>>> This case is also the subject of the patch I submitted back in
>>> July for 86711/86714 and 86552.  With it, GCC avoid folding
>>> the strlen call early and warns for the missing nul:
>>>
>>> warning: ‘__builtin_strlen’ argument missing terminating nul [-Wstringop-overflow=]
>>>      if (__builtin_strlen (u.z) != 7)
>>>          ^~~~~~~~~~~~~~~~
>>>
>>> The patch doesn't doesn't prevent all such strings from being
>>> folded and it eventually lets fold_builtin_strlen() do its thing:
>>>
>>>         /* To avoid warning multiple times about unterminated
>>>            arrays only warn if its length has been determined
>>>            and is being folded to a constant.  */
>>>         if (nonstr)
>>>           warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr);
>>>
>>>         return fold_convert_loc (loc, type, len);
>>>
>>> Handling this case is a matter of avoiding the folding here as
>>> well and moving the warning later.
>>>
>>> Since my patch is still in the review queue and does much more
>>> than just prevent folding of non-nul terminated arrays it should
>>> be reviewed first.
>>>
>>
>> Hmmm, now you made me curious.
>>
>> So I tried to install your patch (I did this on r263508
>> since it does not apply to trunk, one thing I noted is
>> that part 4 and part 3 seem to create gcc/testsuite/gcc.dg/warn-strcpy-no-nul.c
>> I did not check if they are identical or not).
>>
>> So I tried the test case from this PR on the compiler built with your patch:
>>
>> $ cat cat pr87053.c
>> /* PR middle-end/87053 */
>>
>> const union
>> { struct {
>>       char x[4];
>>       char y[4];
>>     };
>>     struct {
>>       char z[8];
>>     };
>> } u = {{"1234", "567"}};
>>
>> int main ()
>> {
>>     if (__builtin_strlen (u.z) != 7)
>>       __builtin_abort ();
>> }
>> $ gcc -S pr87053.c
>> pr87053.c: In function 'main':
>> pr87053.c:15:7: warning: '__builtin_strlen' argument missing terminating nul [-Wstringop-overflow=]
>> 15 |   if (__builtin_strlen (u.z) != 7)
>>      |       ^~~~~~~~~~~~~~~~
>> pr87053.c:11:3: note: referenced argument declared here
>> 11 | } u = {{"1234", "567"}};
>>      |   ^
>> $ cat pr87053.s
>> 	.file	"pr87053.c"
>> 	.text
>> 	.globl	u
>> 	.section	.rodata
>> 	.align 8
>> 	.type	u, @object
>> 	.size	u, 8
>> u:
>> 	.ascii	"1234"
>> 	.string	"567"
>> 	.text
>> 	.globl	main
>> 	.type	main, @function
>> main:
>> .LFB0:
>> 	.cfi_startproc
>> 	pushq	%rbp
>> 	.cfi_def_cfa_offset 16
>> 	.cfi_offset 6, -16
>> 	movq	%rsp, %rbp
>> 	.cfi_def_cfa_register 6
>> 	call	abort
>> 	.cfi_endproc
>> .LFE0:
>> 	.size	main, .-main
>> 	.ident	"GCC: (GNU) 9.0.0 20180813 (experimental)"
>> 	.section	.note.GNU-stack,"",@progbits
>>
>>
>> So we get a warning, and still wrong code.
>>
>> That is the reason why I think this patch of yours adds
>> confusion by trying to fix everything in one step.
>>
>> And I would like you to think of ways how to solve
>> a problem step by step.
>>
>> And at this time, sorry, we should restore correctness issues.
>> And fix wrong-code issues.
>> If possible without breaking existing warnings, yes.
>> But no new warnings, sorry again.
> Just a note, Martin's most fix for 86711/86714 fixes codegen issues
> without breaking existing warnings or adding new warnings.  The new
> warnings were broken out into follow-up patches.
> 

BTW: the warning about u.z not null terminated is bogus.

There are middle-end consistency and correctness issues all over.
They have IMO precedence even over wrong-code issues.


Bernd.

Re: [PATCH] Improve checks in c_strlen (PR 87053)

[Jeff Law]

On 08/22/2018 10:28 AM, Martin Sebor wrote:
> On 08/22/2018 08:41 AM, Bernd Edlinger wrote:
>> Hi!
>>
>>
>> This patch adds some more checks to c_getstr to fix PR middle-end/87053
>> wrong code bug.
>>
>> Unfortunately this patch alone is not sufficient to fix the problem,
>> but also the patch for PR 86714 that hardens c_getstr is necessary
>> to prevent the wrong folding.
>>
>>
>> Bootstrapped and reg-tested on top of my PR 86711/86714 patch.
>> Is it OK for trunk?
> 
> This case is also the subject of the patch I submitted back in
> July for 86711/86714 and 86552.  With it, GCC avoid folding
> the strlen call early and warns for the missing nul:
> 
> warning: ‘__builtin_strlen’ argument missing terminating nul
> [-Wstringop-overflow=]
>    if (__builtin_strlen (u.z) != 7)
>        ^~~~~~~~~~~~~~~~
> 
> The patch doesn't doesn't prevent all such strings from being
> folded and it eventually lets fold_builtin_strlen() do its thing:
> 
>       /* To avoid warning multiple times about unterminated
>          arrays only warn if its length has been determined
>          and is being folded to a constant.  */
>       if (nonstr)
>         warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr);
> 
>       return fold_convert_loc (loc, type, len);
> 
> Handling this case is a matter of avoiding the folding here as
> well and moving the warning later.
> 
> Since my patch is still in the review queue and does much more
> than just prevent folding of non-nul terminated arrays it should
> be reviewed first.
I think we need to address 86711/86714 first.  However, neither approach
to 86711/86714 (Bernd's or yours) is sufficient alone to fix this bug.
This patch can be layered on top of either approach to 86711/86714 to
fix 87053 (I've actually tested that).

So let's table this, hopefully for just a day or so.  It's getting late
and I've got more tests to run on the 86714/86711 patches for comparison
purposes and some loose ends I want to look at in Martin's patch.  I'd
hoped to be finished already, but wasn't able to move things as fast as
I hoped.

jeff

Re: [PATCH] Improve checks in c_strlen (PR 87053)

[Jeff Law]

On 08/22/2018 08:41 AM, Bernd Edlinger wrote:
> Hi!
> 
> 
> This patch adds some more checks to c_getstr to fix PR middle-end/87053
> wrong code bug.
> 
> Unfortunately this patch alone is not sufficient to fix the problem,
> but also the patch for PR 86714 that hardens c_getstr is necessary
> to prevent the wrong folding.
> 
> 
> Bootstrapped and reg-tested on top of my PR 86711/86714 patch.
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.
> 
> 
> 
> patch-pr87053.diff
> 
> 
> gcc:
> 2018-08-22  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	PR middle-end/87053
> 	* builtins.c (c_strlen): Improve range checks.
> 
> testsuite:
> 2018-08-22  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	PR middle-end/87053
> 	* gcc.c-torture/execute/pr87053.c: New test.
This is OK and I'm going to go ahead and commit it momentarily.


>> @@ -700,6 +700,10 @@ c_strlen (tree src, int only_value, unsi
>    unsigned len = string_length (ptr + eltoff * eltsize, eltsize,
>  				strelts - eltoff);
>  
> +  /* Don't know what to return if there was no zero termination.  */
> +  if (len > maxelts - eltoff)
> +    return NULL_TREE;
> +
I'm going to add a comment to the code above indicating that it'd be
nice if this could be an assert in the future.  We're not there yet.

Jeff