genmodes: remove misleading use of strncpy

Richard Guenther wrote:
> On Thu, Apr 19, 2012 at 12:42 PM, Jim Meyering <jim@meyering.net> wrote:
>> Found by inspection.
>> Seeing this strncpy use without the usual following NUL-termination,
>> my reflex was that it was a buffer overrun candidate, but then I
>> realized this is gcc, so that's not very likely.
>> Looking a little harder, I saw the preceding strlen >= sizeof buf test,
>> which means there is no risk of overrun.
>>
>> That preceding test means the use of strncpy is misleading, since
>> with that there is no risk of the name being too long for the buffer,
>> and hence no reason to use strncpy.
>> One way to make the code clearer is to use strcpy, as done below.
>> An alternative is to use memcpy (buf, m->name, strlen (m->name) + 1).

Thanks for the quick feedback.

> Without a comment before the strcpy this isn't any more clear than

Good point.
How about with this folded in?


> when using strncpy.  Also your issue with the terminating NUL is
> still present for the snprintf call done later (in fact the code looks like
> it can be optimized, avoiding the strcpy in the path to the snprintf).

Isn't it different with snprintf?
While strncpy does *NOT* necessarily NUL-terminate,
snprintf explicitly does, always.

IMHO, if you'd like to avoid the duplication in the strcpy/snprintf
path, that deserves to be a different change.

On Thu, Apr 19, 2012 at 2:13 PM, Jim Meyering <jim@meyering.net> wrote:
> Richard Guenther wrote:
>> On Thu, Apr 19, 2012 at 12:42 PM, Jim Meyering <jim@meyering.net> wrote:
>>> Found by inspection.
>>> Seeing this strncpy use without the usual following NUL-termination,
>>> my reflex was that it was a buffer overrun candidate, but then I
>>> realized this is gcc, so that's not very likely.
>>> Looking a little harder, I saw the preceding strlen >= sizeof buf test,
>>> which means there is no risk of overrun.
>>>
>>> That preceding test means the use of strncpy is misleading, since
>>> with that there is no risk of the name being too long for the buffer,
>>> and hence no reason to use strncpy.
>>> One way to make the code clearer is to use strcpy, as done below.
>>> An alternative is to use memcpy (buf, m->name, strlen (m->name) + 1).
>
> Thanks for the quick feedback.
>
>> Without a comment before the strcpy this isn't any more clear than
>
> Good point.
> How about with this folded in?
>
> diff --git a/gcc/genmodes.c b/gcc/genmodes.c
> index f786258..3833017 100644
> --- a/gcc/genmodes.c
> +++ b/gcc/genmodes.c
> @@ -452,6 +452,7 @@ make_complex_modes (enum mode_class cl,
>       if (cl == MODE_FLOAT)
>        {
>          char *p, *q = 0;
> +         /* We verified above that m->name+NUL fits in buf.  */
>          strcpy (buf, m->name);
>          p = strchr (buf, 'F');
>          if (p == 0)

That's better.  Or even cache the strlen result and use memcpy here
as Jakub suggested.

>> when using strncpy.  Also your issue with the terminating NUL is
>> still present for the snprintf call done later (in fact the code looks like
>> it can be optimized, avoiding the strcpy in the path to the snprintf).
>
> Isn't it different with snprintf?
> While strncpy does *NOT* necessarily NUL-terminate,
> snprintf explicitly does, always.

Sure, my point was that the

      if (strlen (m->name) >= sizeof buf)
        {
          error ("%s:%d:mode name \"%s\" is too long",
                 m->file, m->line, m->name);
          continue;
        }

check does not account for the (conditional) prepending of 'C' and the
snprintf would silently discard the last character of the name.

> IMHO, if you'd like to avoid the duplication in the strcpy/snprintf
> path, that deserves to be a different change.

Sure.

The patch is ok with caching strlen and using memcpy.

Richard.

On Thu, Apr 19, 2012 at 02:31:37PM +0200, Richard Guenther wrote:
> That's better.  Or even cache the strlen result and use memcpy here
> as Jakub suggested.

tree-ssa-strlen.c will do that for you when optimizing in this case ;)

	Jakub

On Thu, Apr 19, 2012 at 2:34 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Apr 19, 2012 at 02:31:37PM +0200, Richard Guenther wrote:
>> That's better.  Or even cache the strlen result and use memcpy here
>> as Jakub suggested.
>
> tree-ssa-strlen.c will do that for you when optimizing in this case ;)

Does it?  But only if you used strcpy, no?

Richard.

>        Jakub

On Thu, Apr 19, 2012 at 02:38:11PM +0200, Richard Guenther wrote:
> On Thu, Apr 19, 2012 at 2:34 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Thu, Apr 19, 2012 at 02:31:37PM +0200, Richard Guenther wrote:
> >> That's better.  Or even cache the strlen result and use memcpy here
> >> as Jakub suggested.
> >
> > tree-ssa-strlen.c will do that for you when optimizing in this case ;)
> 
> Does it?  But only if you used strcpy, no?

Yes.  I don't think it would handle the strncpy, it is in that case not
about caching the value, but also additional comparison and knowing it is
sufficiently large.

	Jakub