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

[Jeff Law]

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.
This hunk is gone in the V2 patch.  So I'm not going to worry about it
right now.



> 
>> +  if (!nulterm && string[string_size - 1] != '\0')
>>      {
>> -      /* Support only properly NUL-terminated strings but handle
>> -	 consecutive strings within the same array, such as the six
>> -	 substrings in "1\0002\0003".  */
>> +      /* When NULTERM is null, support only properly nul-terminated
>> +	 strings but handle consecutive strings within the same array,
>> +	 such as the six substrings in "1\0002\0003".  Otherwise, let
>> +	 the caller deal with non-nul-terminated arrays.  */
>>        return NULL;
>>      }
>>  
>> -  return offset <= string_length ? string + offset : "";
> this should be offset < string_size.
> 
>> +  return offset <= string_size ? string + offset : "";
Agreed.

jeff