Richard Guenther wrote:
> On Thu, Apr 19, 2012 at 2:13 PM, Jim Meyering <j...@meyering.net> wrote:
>> Richard Guenther wrote:
>>> On Thu, Apr 19, 2012 at 12:42 PM, Jim Meyering <j...@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.
Yes, you're right.
It's good to avoid snprintf and its truncation, too.
>> 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.
Like this, I presume:
[alternatively, declare and compute m_len on a separate line,
just before the comparison:
size_t m_len = strlen (m->name);
I'd actually prefer that, but don't know if decl-after-stmt is ok here.
]
2012-04-19 Jim Meyering <meyer...@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 memcpy.
diff --git a/gcc/genmodes.c b/gcc/genmodes.c
index 8b6f5bc..484a6ac 100644
--- a/gcc/genmodes.c
+++ b/gcc/genmodes.c
@@ -1,5 +1,5 @@
/* 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.
@@ -435,11 +435,13 @@ make_complex_modes (enum mode_class cl,
for (m = modes[cl]; m; m = m->next)
{
+ size_t m_len;
+
/* Skip BImode. FIXME: BImode probably shouldn't be MODE_INT. */
if (m->precision == 1)
continue;
- if (strlen (m->name) >= sizeof buf)
+ if ((m_len = strlen (m->name)) >= sizeof buf)
{
error ("%s:%d:mode name \"%s\" is too long",
m->file, m->line, m->name);
@@ -452,7 +454,8 @@ make_complex_modes (enum mode_class cl,
if (cl == MODE_FLOAT)
{
char *p, *q = 0;
- strncpy (buf, m->name, sizeof buf);
+ /* We verified above that m->name+NUL fits in buf. */
+ memcpy (buf, m->name, m_len + 1);
p = strchr (buf, 'F');
if (p == 0)
q = strchr (buf, 'D');
--
1.7.10.208.gb4267
