| Message ID | 871unjq3is.fsf@rho.meyering.net |
|---|---|
| State | New |
| Headers | show |
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
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.
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');