This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]