Patchwork genmodes: don't truncate a mode name of length >= 7

login
register
mail settings
Submitter Jim Meyering
Date April 19, 2012, 8:35 p.m.
Message ID <8762cvmpd0.fsf_-_@rho.meyering.net>
Download mbox | patch
Permalink /patch/153868/
State New
Headers show

Comments

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

Here's a patch for that.  (passes make bootstrap)
Incidentally it removes the name-length limitation of 7 and it hoists
the xmalloc, which induces the single added free-before-continue.
There was some clean-up along the way: "q" was easy to eliminate, and
I pulled the two identical snprintf calls out of the "if" block and
replaced them with the buf[0] assignment and memcpy.


2012-04-19  Jim Meyering  <meyering@redhat.com>

	* genmodes.c (make_complex_modes): Don't truncate a mode name of
        length 7 or more when prepending a "C".  Suggested by Richard Guenther.


 gcc/genmodes.c |   32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

--
1.7.10.208.gb4267
Richard Guenther - April 20, 2012, 8:46 a.m.
On Thu, Apr 19, 2012 at 10:35 PM, Jim Meyering <jim@meyering.net> wrote:
> Richard Guenther wrote:
>> 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.
>
> Here's a patch for that.  (passes make bootstrap)
> Incidentally it removes the name-length limitation of 7 and it hoists
> the xmalloc, which induces the single added free-before-continue.
> There was some clean-up along the way: "q" was easy to eliminate, and
> I pulled the two identical snprintf calls out of the "if" block and
> replaced them with the buf[0] assignment and memcpy.

Ok.

Thanks,
Richard.

>
> 2012-04-19  Jim Meyering  <meyering@redhat.com>
>
>        * genmodes.c (make_complex_modes): Don't truncate a mode name of
>        length 7 or more when prepending a "C".  Suggested by Richard Guenther.
>
>
>  gcc/genmodes.c |   32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/gcc/genmodes.c b/gcc/genmodes.c
> index 6bbeb6f..84517b9 100644
> --- a/gcc/genmodes.c
> +++ b/gcc/genmodes.c
> @@ -427,7 +427,6 @@ make_complex_modes (enum mode_class cl,
>  {
>   struct mode_data *m;
>   struct mode_data *c;
> -  char buf[8];
>   enum mode_class cclass = complex_class (cl);
>
>   if (cclass == MODE_RANDOM)
> @@ -435,6 +434,7 @@ make_complex_modes (enum mode_class cl,
>
>   for (m = modes[cl]; m; m = m->next)
>     {
> +      char *p, *buf;
>       size_t m_len;
>
>       /* Skip BImode.  FIXME: BImode probably shouldn't be MODE_INT.  */
> @@ -442,40 +442,34 @@ make_complex_modes (enum mode_class cl,
>        continue;
>
>       m_len = strlen (m->name);
> -      if (m_len >= sizeof buf)
> -       {
> -         error ("%s:%d:mode name \"%s\" is too long",
> -                m->file, m->line, m->name);
> -         continue;
> -       }
> +      /* The leading "1 +" is in case we prepend a "C" below.  */
> +      buf = (char *) xmalloc (1 + m_len + 1);
>
>       /* Float complex modes are named SCmode, etc.
>         Int complex modes are named CSImode, etc.
>          This inconsistency should be eliminated.  */
> +      p = 0;
>       if (cl == MODE_FLOAT)
>        {
> -         char *p, *q = 0;
> -         /* 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');
> -         if (p == 0 && q == 0)
> +         if (p == 0 && strchr (buf, 'D') == 0)
>            {
>              error ("%s:%d: float mode \"%s\" has no 'F' or 'D'",
>                     m->file, m->line, m->name);
> +             free (buf);
>              continue;
>            }
> -
> -         if (p != 0)
> -           *p = 'C';
> -         else
> -           snprintf (buf, sizeof buf, "C%s", m->name);
>        }
> +      if (p != 0)
> +       *p = 'C';
>       else
> -       snprintf (buf, sizeof buf, "C%s", m->name);
> +       {
> +         buf[0] = 'C';
> +         memcpy (buf + 1, m->name, m_len + 1);
> +       }
>
> -      c = new_mode (cclass, xstrdup (buf), file, line);
> +      c = new_mode (cclass, buf, file, line);
>       c->component = m;
>     }
>  }
> --
> 1.7.10.208.gb4267
Jim Meyering - April 20, 2012, 3:38 p.m.
Richard Guenther wrote:
> On Thu, Apr 19, 2012 at 10:35 PM, Jim Meyering <jim@meyering.net> wrote:
>> Richard Guenther wrote:
>>> 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.
>>
>> Here's a patch for that.  (passes make bootstrap)
>> Incidentally it removes the name-length limitation of 7 and it hoists
>> the xmalloc, which induces the single added free-before-continue.
>> There was some clean-up along the way: "q" was easy to eliminate, and
>> I pulled the two identical snprintf calls out of the "if" block and
>> replaced them with the buf[0] assignment and memcpy.
>
> Ok.

Thanks for the review.  Committed.

Patch

diff --git a/gcc/genmodes.c b/gcc/genmodes.c
index 6bbeb6f..84517b9 100644
--- a/gcc/genmodes.c
+++ b/gcc/genmodes.c
@@ -427,7 +427,6 @@  make_complex_modes (enum mode_class cl,
 {
   struct mode_data *m;
   struct mode_data *c;
-  char buf[8];
   enum mode_class cclass = complex_class (cl);

   if (cclass == MODE_RANDOM)
@@ -435,6 +434,7 @@  make_complex_modes (enum mode_class cl,

   for (m = modes[cl]; m; m = m->next)
     {
+      char *p, *buf;
       size_t m_len;

       /* Skip BImode.  FIXME: BImode probably shouldn't be MODE_INT.  */
@@ -442,40 +442,34 @@  make_complex_modes (enum mode_class cl,
 	continue;

       m_len = strlen (m->name);
-      if (m_len >= sizeof buf)
-	{
-	  error ("%s:%d:mode name \"%s\" is too long",
-		 m->file, m->line, m->name);
-	  continue;
-	}
+      /* The leading "1 +" is in case we prepend a "C" below.  */
+      buf = (char *) xmalloc (1 + m_len + 1);

       /* Float complex modes are named SCmode, etc.
 	 Int complex modes are named CSImode, etc.
          This inconsistency should be eliminated.  */
+      p = 0;
       if (cl == MODE_FLOAT)
 	{
-	  char *p, *q = 0;
-	  /* 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');
-	  if (p == 0 && q == 0)
+	  if (p == 0 && strchr (buf, 'D') == 0)
 	    {
 	      error ("%s:%d: float mode \"%s\" has no 'F' or 'D'",
 		     m->file, m->line, m->name);
+	      free (buf);
 	      continue;
 	    }
-
-	  if (p != 0)
-	    *p = 'C';
-	  else
-	    snprintf (buf, sizeof buf, "C%s", m->name);
 	}
+      if (p != 0)
+	*p = 'C';
       else
-	snprintf (buf, sizeof buf, "C%s", m->name);
+	{
+	  buf[0] = 'C';
+	  memcpy (buf + 1, m->name, m_len + 1);
+	}

-      c = new_mode (cclass, xstrdup (buf), file, line);
+      c = new_mode (cclass, buf, file, line);
       c->component = m;
     }
 }