Re: [PATCH] genmodes: remove misleading use of strncpy

[Richard Guenther <richard dot guenther at gmail dot com>]

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).
>
> Ok to commit?

Without a comment before the strcpy this isn't any more clear than
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).

strncpy -> strcpy definitely looks like premature optimization to me.

Richard.

> 2012-04-19 ?Jim Meyering ?<meyering@redhat.com>
>
> ? ? ? ?genmodes: remove misleading use of strncpy
> ? ? ? ?* genmodes.c (make_complex_modes): Avoid unnecessary use of strncpy.
> ? ? ? ?We verified above that the string(including trailing NUL) fits in buf,
> ? ? ? ?so just use strcpy.
>
>
> diff --git a/gcc/genmodes.c b/gcc/genmodes.c
> index 8b6f5bc..f786258 100644
> --- a/gcc/genmodes.c
> +++ b/gcc/genmodes.c
> @@ -1,15 +1,15 @@
> ?/* Generate the machine mode enumeration and associated tables.
> - ? Copyright (C) 2003, 2004, 2005, 2006, 2007, 2010
> + ? Copyright (C) 2003, 2004, 2005, 2006, 2007, 2010, 2012
> ? ?Free Software Foundation, Inc.
>
> ?This file is part of GCC.
>
> ?GCC is free software; you can redistribute it and/or modify it under
> ?the terms of the GNU General Public License as published by the Free
> ?Software Foundation; either version 3, or (at your option) any later
> ?version.
>
> ?GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> ?WARRANTY; without even the implied warranty of MERCHANTABILITY or
> ?FITNESS FOR A PARTICULAR PURPOSE. ?See the GNU General Public License
> ?for more details.
> @@ -442,27 +442,27 @@ make_complex_modes (enum mode_class cl,
> ? ? ? if (strlen (m->name) >= sizeof buf)
> ? ? ? ?{
> ? ? ? ? ?error ("%s:%d:mode name \"%s\" is too long",
> ? ? ? ? ? ? ? ? m->file, m->line, m->name);
> ? ? ? ? ?continue;
> ? ? ? ?}
>
> ? ? ? /* Float complex modes are named SCmode, etc.
> ? ? ? ? Int complex modes are named CSImode, etc.
> ? ? ? ? ?This inconsistency should be eliminated. ?*/
> ? ? ? if (cl == MODE_FLOAT)
> ? ? ? ?{
> ? ? ? ? ?char *p, *q = 0;
> - ? ? ? ? strncpy (buf, m->name, sizeof buf);
> + ? ? ? ? strcpy (buf, m->name);
> ? ? ? ? ?p = strchr (buf, 'F');
> ? ? ? ? ?if (p == 0)
> ? ? ? ? ? ?q = strchr (buf, 'D');
> ? ? ? ? ?if (p == 0 && q == 0)
> ? ? ? ? ? ?{
> ? ? ? ? ? ? ?error ("%s:%d: float mode \"%s\" has no 'F' or 'D'",
> ? ? ? ? ? ? ? ? ? ? m->file, m->line, m->name);
> ? ? ? ? ? ? ?continue;
> ? ? ? ? ? ?}
>
> ? ? ? ? ?if (p != 0)
> ? ? ? ? ? ?*p = 'C';
> ? ? ? ? ?else
> --
> 1.7.10.208.gb4267