[PING**3] [PATCH] Fix not properly nul-terminated string constants in JIT

[PING**3] [PATCH] Fix not properly nul-terminated string constants in JIT

[Bernd Edlinger]

Ping...

On 10/10/18 9:43 PM, Bernd Edlinger wrote:
> Ping...
> 
> 
> On 08/26/18 21:40, Bernd Edlinger wrote:
>> Ping...
>>
>> This is just plain wrong, independent
>> of any STRING_CST semantic issues.
>>
>> The original patch (retested on current trunk) is
>> here: https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00370.html
>>
>>
>> On 08/05/18 18:59, 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?
>>>
>>>
>>> Thanks
>>> Bernd.

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

[Bernd Edlinger]

Sorry, Dave,


what should I do with this patch?

Bernd.

On 11/9/18 5:52 PM, Bernd Edlinger wrote:
> 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
>>>