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

[David Malcolm <dmalcolm at redhat dot com>]

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.

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?

> @@ -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.

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?

Thanks
Dave