Re: [PING] [PATCH] Fix wrong code with truncated string literals (PR 86711/86714)

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

On 08/18/18 06:01, Jeff Law wrote:
> On 08/17/2018 03:14 AM, Bernd Edlinger wrote:
>> Hi!
>>
>>
>> After the other patch has been applied, I re-based this patch accordingly.
>>
>> Except the mechanical changes, there are a few notable differences to the
>> previous version:
>>
>> In string_constant, I added a similar check for the STRING_CSTs
>> because when callers don't use mem_size, they assume to be
>> able to read "TREE_STRING_LENGTH (array)" bytes, but that is
>> not always the case, for languages that don't always use
>> zero-terminated strings, for instance hollerith strings in fortran.
>>
>> --- gcc/expr.c  2018-08-17 05:32:57.332211963 +0200
>> +++ gcc/expr.c  2018-08-16 23:08:23.544940795 +0200
>> @@ -11372,6 +11372,9 @@ string_constant (tree arg, tree *ptr_off
>>         *ptr_offset = fold_convert (sizetype, offset);
>>         if (mem_size)
>>          *mem_size = TYPE_SIZE_UNIT (TREE_TYPE (array));
>> +      else if (compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (array)),
>> +                                TREE_STRING_LENGTH (array)) < 0)
>> +       return NULL_TREE;
>>         return array;
>>       }
>>
> Yes.  I purposefully didn't include this change in its entirety as it
> was dependent upon your earlier patch.   Instead I twiddled your patch
> so that it applied on the current trunk and didn't regress anything
> while keeping the core concept you were introducing.
> 

That's a more or less theoretical possibility that I just
considered useful for symmetry, and maybe Ada/Go strings.

So it should be completely impossible to have this change anything
in C (hopefully!!!).

If it would happen, that would probably be a bug that needs
to be fixed.

> 
> I'm also confident that change breaks one or more tests in the
> testsuite.  I'm not sure why you didn't see the regression.   But I
> certainly saw it with the variant shown above.
> 

I tested the patch against an older version of the trunk,
but today, I repeated the regression test against r263644.

This time I attach the test results for your reference:
gcc-trunk-263644-test.txt: unchanged r263644
gcc-trunk-263644-0-test.txt: r263644 + this patch
gcc-trunk-263644-1-test.txt: r263644 + this patch without string_constant

As you can see, these regressions are already in r263644:
FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O0  execution test
FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2  execution test
FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O0  execution test
FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2  execution test
FAIL: c-c++-common/torture/builtin-arith-overflow-p-17.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
FAIL: gcc.target/i386/20040112-1.c scan-assembler testb

I have never seen those before, so they are brand new.

FAIL: gnat.dg/dinst.adb scan-assembler loc [0-9] 5 [0-9]( is_stmt [0-9])? discriminator 1\\n
FAIL: gnat.dg/dinst.adb scan-assembler loc [0-9] 5 [0-9]( is_stmt [0-9])? discriminator 3\\n
FAIL: gnat.dg/dinst.adb scan-assembler loc [0-9] 5 [0-9]( is_stmt [0-9])? discriminator 4\\n

these were already there on monday, but not last week.

The test result with this patch as is does not change anything.

But what's surprising, is this:

--- gcc-trunk-r263644-test.txt	2018-08-18 13:49:27.214851609 +0200
+++ gcc-trunk-r263644-1-test.txt	2018-08-18 13:49:27.070851601 +0200
@@ -18,6 +18,12 @@
  
  
  Running target unix
+FAIL: gcc.c-torture/execute/pr86714.c   -O1  execution test
+FAIL: gcc.c-torture/execute/pr86714.c   -O2  execution test
+FAIL: gcc.c-torture/execute/pr86714.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
+FAIL: gcc.c-torture/execute/pr86714.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  execution test
+FAIL: gcc.c-torture/execute/pr86714.c   -O3 -g  execution test
+FAIL: gcc.c-torture/execute/pr86714.c   -Os  execution test
  FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O0  execution test
  FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2  execution test
  FAIL: c-c++-common/torture/builtin-arith-overflow-17.c   -O2 -flto -fno-use-linker-plugin -flto-partiti

There is another regression in gcc-trunk-r263644-1-test.txt only which is likely only noise
that happens randomly and can be ignored:

                 === libgo Summary for unix ===

# of expected passes            163

Running target unix/-m32
FAIL: sync


The result is very interesting, and is probably what you were looking for:

The test was run with both test cases, but only pr86714 failed, so the change in c_getstr
Fixes pr86711, while the change in string_constant fixes pr86714 (or both).

The patch could actually be split between pr86711 and pr86714.

The c_getstr / p86711 should be applied before
string_constant / pr86714.


Thanks
Bernd.

> Anyway, the basic idea was to carve out the basic concept of your patch
> to allow callers to specify how to count without regressing the trunk.
> 
> That allows us to carve out an issue, resolve it and move on.  The
> interdependent and conflicting patches are a nightmare to sort out.
> 
>>
>> The range check in c_getstr was refined as well:
>>
>> This I added, because vla arrays can be initialized with string constants,
>> especially since the 71625 patch was installed:
>> In this case we end up with mem_size that fails to be constant.
>>
>>
>> @@ -14606,25 +14603,17 @@ c_getstr (tree src, unsigned HOST_WIDE_I
>>          offset = tree_to_uhwi (offset_node);
>>       }
>>
>> +  if (!tree_fits_uhwi_p (mem_size))
>> +    return NULL;
>> +
>>     /* STRING_LENGTH is the size of the string literal, including any
>>        embedded NULs.  STRING_SIZE is the size of the array the string
>>        literal is stored in.  */
>>
>> Also the rest of the string length checks are refined, to return
>> actually zero-terminated single byte strings when strlen is not given,
>> and return something not necessarily zero-terminated which is
>> suitable for memxxx-functions otherwise.
> 
>>
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
> Not yet.  There's a lot to go over here and I haven't finished reviewing
> all the discussions around 86711/86714.
> 
> 
> Jeff
>

Attachment: gcc-trunk-r263644-0-test.txt
Description: gcc-trunk-r263644-0-test.txt

Attachment: gcc-trunk-r263644-1-test.txt
Description: gcc-trunk-r263644-1-test.txt

Attachment: gcc-trunk-r263644-test.txt
Description: gcc-trunk-r263644-test.txt