Patchwork genmodes: remove misleading use of strncpy

login
register
mail settings
Submitter Jim Meyering
Date April 19, 2012, 1:01 p.m.
Message ID <871unjq3is.fsf@rho.meyering.net>
Download mbox | patch
Permalink /patch/153748/
State New
Headers show

Comments

Jim Meyering - April 19, 2012, 1:01 p.m.
Richard Guenther wrote:
> 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.

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

--
1.7.10.208.gb4267
Richard Guenther - April 19, 2012, 1:40 p.m.
On Thu, Apr 19, 2012 at 3:01 PM, Jim Meyering <jim@meyering.net> wrote:
> Richard Guenther wrote:
>> 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.
>
> 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.
>  ]

Works for me with ...

> 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 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)

... the strlen in a separate stmt like

        m_len = strlen (m->name);
        if (m_len >= sizeof (buf))

Thanks,
Richard.

>        {
>          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
Jim Meyering - April 19, 2012, 1:52 p.m.
Richard Guenther wrote:
...
>>> 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.
>>  ]
>
> Works for me with ...
...
>> -      if (strlen (m->name) >= sizeof buf)
>> +      if ((m_len = strlen (m->name)) >= sizeof buf)
>
> ... the strlen in a separate stmt like
>
>         m_len = strlen (m->name);
>         if (m_len >= sizeof (buf))

Thanks.
Adjusted and committed.

Patch

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');