Message ID | 87fwbzq5qr.fsf@rho.meyering.net |
---|---|
State | New |
Headers | show |
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. > 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. Richard.
On Thu, Apr 19, 2012 at 02:31:37PM +0200, Richard Guenther wrote: > That's better. Or even cache the strlen result and use memcpy here > as Jakub suggested. tree-ssa-strlen.c will do that for you when optimizing in this case ;) Jakub
On Thu, Apr 19, 2012 at 2:34 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Apr 19, 2012 at 02:31:37PM +0200, Richard Guenther wrote: >> That's better. Or even cache the strlen result and use memcpy here >> as Jakub suggested. > > tree-ssa-strlen.c will do that for you when optimizing in this case ;) Does it? But only if you used strcpy, no? Richard. > Jakub
On Thu, Apr 19, 2012 at 02:38:11PM +0200, Richard Guenther wrote: > On Thu, Apr 19, 2012 at 2:34 PM, Jakub Jelinek <jakub@redhat.com> wrote: > > On Thu, Apr 19, 2012 at 02:31:37PM +0200, Richard Guenther wrote: > >> That's better. Or even cache the strlen result and use memcpy here > >> as Jakub suggested. > > > > tree-ssa-strlen.c will do that for you when optimizing in this case ;) > > Does it? But only if you used strcpy, no? Yes. I don't think it would handle the strncpy, it is in that case not about caching the value, but also additional comparison and knowing it is sufficiently large. Jakub
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)