PING [PATCH] warn for strlen of arrays with missing nul (PR 86552, 86711, 86714) )

[Bernd Edlinger]

On 08/24/18 18:51, Jeff Law wrote:
> On 08/02/2018 07:26 AM, Bernd Edlinger wrote:
>>
>>> -  if (compare_tree_int (array_size, length + 1) < 0)
>>> +  if (nulterm)
>> but here you compare bytes with length which is measued un chars.
>>
>>> +    *nulterm = array_elts > length;
>>> +  else if (array_elts <= length)
>>>       return NULL_TREE;
>>>   
>>>     *ptr_offset = offset;
> Actually in the V2 patch length is measured by element size.  So I think
> this is a non-issue.
> 
> 
> 
>>> -      /* Compute and store the length of the substring at OFFSET.
>>> +      /* Compute and store the size of the substring at OFFSET.
>>>   	 All offsets past the initial length refer to null strings.  */
>>> -      if (offset <= string_length)
>>> -	*strlen = string_length - offset;
>> this should be offset < string_length.
>>
>>> +      if (offset <= string_size)
>>> +	*strsize = string_size - offset;
>>>         else
>>> -	*strlen = 0;
>> this should be 1, you may access the NUL byte of "".
>>
>>> +	*strsize = 0;
>>>       }
> Agreed in both cases based on the defined behavior for the function (NUL
> is included).
> 
> 
>>>   
>>> -  const char *string = TREE_STRING_POINTER (src);
>>> -
>>> -  if (string_length == 0
>>> -      || offset >= string_size)
>>> +  if (string_size == 0
>>> +      || offset >= array_size)
>>>       return NULL;
>>>   
>>> -  if (strsize)
>>> -    {
>>> -      /* Support even constant character arrays that aren't proper
>>> -	 NUL-terminated strings.  */
>>> -      *strsize = string_size;
>>> -    }
>>> -  else if (string[string_length - 1] != '\0')
>> Well, this is broken for wide character strings.
>> but I hope we can get rid of STRING_CST which are
>> not explicitly null terminated.

I am afraid that is not going to happen.
Maybe we can get STRING_CST that are never longer
than the TYPE_UNIT_SIZE, but c_strlen and c_getstr
need to take care that the string is zero-terminated.

string_constant, should not promise the string is zero terminated.
But instead it can promise that:
1) the STRING_CST is valid up to TREE_STRING_LENGTH
2) mem_size is >= TREE_STRING_LENGTH
3) memory between TREE_STRING_LENGTH and mem_size is ZERO.

It will not guarantee anything about zero termination any more.

But that will again need an adjustment to the string_constant
and likely to my 86711/86714/87053 patch set.

Sorry for all this back and forth.

> This hunk is gone in the V2 patch.  So I'm not going to worry about it
> right now.
> 
> 

Hmm.  In my tree I wanted to drop this check first, but that
did not work right. So I skip the check if strsize is not NULL.
But if it is NULL, I check the element size is 1, if it is
not 1 return NULL, because it is no character string.

I had several iterations here, and it might still be dependent
on semantical properties of STRING_CST that may not be be granted
in the light of my recent discoveries.


In the end, the best approach might be to either merge my patch
with Martins, or step-wise, first fixing wrong code, and then
implementing warnings without fixing wrong code.

Anyway, the smaller the patch the better for the review.


Bernd.