Re: [PATCH] Fix not properly nul-terminated string constants in JIT

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

Hi Dave,

is the patch OK, or do you still have questions?


Thanks
Bernd.

On 11/2/18 10:48 PM, Bernd Edlinger wrote:
> On 11/2/18 9:40 PM, David Malcolm wrote:
>> On Sun, 2018-08-05 at 16:59 +0000, Bernd Edlinger wrote:
>>> Hi!
>>>
>>>
>>> My other patch with adds assertions to varasm.c regarding correct
>>> nul termination of sting literals did make these incorrect string
>>> constants in JIT frontend fail.
>>>
>>> The string constants are not nul terminated if their length exceeds
>>> 200 characters.  The test cases do not use strings of that size where
>>> that would make a difference.  But using a fixed index type is
>>> clearly
>>> wrong.
>>>
>>> This patch removes the fixed char[200] array type from
>>> playback::context,
>>> and uses build_string_literal instead of using build_string directly.
>>>
>>>
>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>> Is it OK for trunk?
>>
>> Sorry for the belated response.
>>
>> Was this tested with --enable-host-shared and --enable-languages=jit ?
>> Note that "jit" is not included in --enable-languages=all.
>>
> 
> Yes, of course.  The test suite contains a few string constants, just
> all of them are shorter than 200 characters.  But I think removing this
> artificial limit enables the existing test cases to test that the
> shorter string is in fact zero terminated.
> 
>> The patch seems reasonable, but I'm a little confused over the meaning
>> of "len" in build_string_literal and build_string: does it refer to the
>> length or the size of the string?
>>
> 
> build_string_literal:
> For languages that use zero-terminated strings, len is strlen(str)+1, and
> str is a zero terminated single-byte character string.
> For languages that don't use zero-terminated strings, len is the size of
> the string and str is not zero terminated.
> 
> build_string:
> constructs a STRING_CST tree object, which is usable as is in some contexts,
> like for asm constraints, but as a string literal it is incomplete, and
> needs an index type.  The index type defines the memory size which must
> be larger than the string precision.  Excess memory is implicitly cleared.
> 
> This means currently all jit strings shorter than 200 characters
> are filled with zero up to the limit of 200 chars as imposed by
> m_char_array_type_node.  Strings of exactly 200 chars are not zero terminated,
> and larger strings should result in an assertion (excess precision was previously
> allowed, but no zero termination was appended, when that is not part of
> the original string constant).
> 
> Previously it was allowed to have memory size less than the string len, which
> had complicated the STRING_CST semantics in the middle-end, but with the
> string_cst semantic rework I did for gcc-9 this is no longer allowed and
> results in (checking) assertions in varasm.c.
> 
>>> @@ -617,16 +616,9 @@ playback::rvalue *
>>>   playback::context::
>>>   new_string_literal (const char *value)
>>>   {
>>> -  tree t_str = build_string (strlen (value), value);
>>> -  gcc_assert (m_char_array_type_node);
>>> -  TREE_TYPE (t_str) = m_char_array_type_node;
>>> -
>>> -  /* Convert to (const char*), loosely based on
>>> -     c/c-typeck.c: array_to_pointer_conversion,
>>> -     by taking address of start of string.  */
>>> -  tree t_addr = build1 (ADDR_EXPR, m_const_char_ptr, t_str);
>>> +  tree t_str = build_string_literal (strlen (value) + 1, value);
>>> -  return new rvalue (this, t_addr);
>>> +  return new rvalue (this, t_str);
>>>   }
>>
>> In the above, the call to build_string with strlen is replaced with
>> build_string_literal with strlen + 1.
>>
>>
>> build_string's comment says:
>>
>> "Note that for a C string literal, LEN should include the trailing
>> NUL."
>>
>> but has:
>>
>>    length = len + offsetof (struct tree_string, str) + 1;
>>
>> and:
>>
>>    TREE_STRING_LENGTH (s) = len;
>>    memcpy (s->string.str, str, len);
>>    s->string.str[len] = '\0';
>>
>> suggesting that the "len" parameter is in fact the length *without* the
>> trailing NUL, and that a trailing NUL is added by build_string.
>>
> 
> Yes, string constants in tree objects have another zero termiation,
> but varasm.c does something different, there the index range takes
> precedence.
> The index range is built in build_string_literal as follows:
> 
>    elem = build_type_variant (char_type_node, 1, 0);
>    index = build_index_type (size_int (len - 1));
>    type = build_array_type (elem, index);
> 
> therefore the string constant hast the type char[0..len-1]
> thus only len bytes are significant for code generation, the extra
> nul is just for "convenience".
> 
>> However build_string_literal has:
>>
>>    t = build_string (len, str);
>>    elem = build_type_variant (char_type_node, 1, 0);
>>    index = build_index_type (size_int (len - 1));
>>
>> suggesting that the len is passed directly to build_string (and thus
>> ought to be strlen), but the build_index_type uses len - 1 (which
>> suggests that len is the size of the string, rather than its length).
>>
>> What's the intended meaning of len in these functions?
>>
> 
> I hope this helps.
> 
> 
> Thanks
> Bernd.
> 
> 
>> Thanks
>> Dave
>>