Re: PING [PATCH] warn for strlen of arrays with missing nul (PR 86552)

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

On 07/31/18 05:51, Martin Sebor wrote:
> On 07/30/2018 03:11 PM, Bernd Edlinger wrote:
>> Hi,
>>
>>> @@ -621,6 +674,12 @@ c_strlen (tree src, int only_value)
>>>     maxelts = maxelts / eltsize - 1;
>>>       }
>>>
>>> +  /* Unless the caller is prepared to handle it by passing in a non-null
>>> +     ARR, fail if the terminating nul doesn't fit in the array the string
>>> +     is stored in (as in const char a[3] = "123";  */
>>> +  if (!arr && maxelts < strelts)
>>> +    return NULL_TREE;
>>> +
>>
>> this is c_strlen, how is the caller ever supposed to handle non-zero terminated strings???
>> especially if you do this above?
> 
> Callers that pass in a non-null ARR handle them by issuing
> a warning.  The rest get back a null result.  It should be
> evident from the rest of the patch.  It can be debated what
> each caller should do when it detects such a missing nul
> where one is expected.  Different approaches may be more
> or less appropriate for different callers/functions (e.g.,
> strcpy vs strlen).
> 

Sorry, right in the beginning you have "if (!add) arr = arrs;"

>>> +c_strlen (tree src, int only_value, tree *arr /* = NULL */)
>>> {
>>>   STRIP_NOPS (src);
>>> +
>>> +  /* Used to detect non-nul-terminated strings in subexpressions
>>> +     of a conditional expression.  When ARR is null, point it at
>>> +     one of the elements for simplicity.  */
>>> +  tree arrs[] = { NULL_TREE, NULL_TREE };
>>> +  if (!arr)
>>> +    arr = arrs;
>>
>>> @@ -11427,7 +11478,9 @@ string_constant (tree arg, tree *ptr_offset)
>>>   unsigned HOST_WIDE_INT length = TREE_STRING_LENGTH (init);
>>>   length = string_length (TREE_STRING_POINTER (init), charsize,
>>>               length / charsize);
>>> -  if (compare_tree_int (array_size, length + 1) < 0)
>>> +  if (nulterm)
>>> +    *nulterm = array_elts > length;
>>> +  else if (array_elts <= length)
>>>     return NULL_TREE;
>>
>> I don't understand why you can't use
>> compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (init)), TREE_STRING_LENGTH (init))
>> instead of this convoluted code above ???
>>
>> Sorry, this patch does not look like it is ready any time soon.
> 
> I'm open to technical comments on the substance of my changes
> but I'm not interested in your opinion of the readiness of
> the patch (whatever that might mean), certainly not if you
> have formed it after skimming a random handful of lines out
> of a 600 line patch.
> 

Sorry, again.  I just meant you should fix the issues, and
maybe make the patch a bit smaller.

>> But actually I am totally puzzled by your priorities.
>> This is what I see right now:
>>
>> 1) We have missing warnings.
>> 2) We have wrong code bugs.
>> 3) We have apparently a specification error on the C Language standard (*)
>>
>>
>> Why are you prioritizing 1) over 2) thus blocking my attempts to fix a wrong code
>> issue,and why do you not tackle 3) in your WG14?
> 
> My priorities are none of your concern.
> 

Sorry, again, but your priorities seem to conflict with mine.

> Your "attempts to fix" issues interfere with my work on a number
> of projects.  You are not being helpful -- instead, by submitting
> changes that you know fully well conflict with mine, you are
> impeding and undermining my work.  That is why I object to them.
> 
>> (*) which means that GCC is currently removing code from assertions
>> as I pointed out here: https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01695.html
>>
>> This happens because GCC follows the language standards literally right now.
>>
>> I would say too literally, and it proves that the language standard's logic is
>> flawed IMHO.
> 
> I have no idea what your point is about standards, but bugs
> like the one in the example, including those arising from
> uninitialized arrays, could be detected with only minor
> enhancements to the tree-ssa-strlen pass.  Implementing some
> of this is among the projects I'm expected and expecting to
> work on for GCC 9.  This patch is a small step in that
> direction.
> 
> If you care about detecting bugs I would expect you to be
> supportive rather than dismissive of this work, and helpful
> in bringing it to fruition rather that putting it down or
> questioning my priorities.  Especially since the work was
> prompted by your own (valid) complaint that GCC doesn't
> diagnose them.
> 

You don't really listen to what I am saying, I did not say
that we need another warning instead of fixing the wrong
optimization issue at hand.

But I am in good company, you don't listen to Jakub and Richi
either.


Bernd.

> Martin
>