Re: [PATCH] genmodes: remove misleading use of strncpy
On Thu, Apr 19, 2012 at 12:42 PM, Jim Meyering j...@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).
Ok to commit?
Without a comment before the strcpy this isn't any more clear than
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).
strncpy - strcpy definitely looks like premature optimization to me.
Richard.
2012-04-19 Jim Meyering meyer...@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 strcpy.
diff --git a/gcc/genmodes.c b/gcc/genmodes.c
index 8b6f5bc..f786258 100644
--- a/gcc/genmodes.c
+++ b/gcc/genmodes.c
@@ -1,15 +1,15 @@
/* 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.
GCC is free software; you can redistribute it and/or modify it under
the terms of the GNU General Public License as published by the Free
Software Foundation; either version 3, or (at your option) any later
version.
GCC is distributed in the hope that it will be useful, but WITHOUT ANY
WARRANTY; without even the implied warranty of MERCHANTABILITY or
FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
for more details.
@@ -442,27 +442,27 @@ make_complex_modes (enum mode_class cl,
if (strlen (m-name) = sizeof buf)
{
error (%s:%d:mode name \%s\ is too long,
m-file, m-line, m-name);
continue;
}
/* Float complex modes are named SCmode, etc.
Int complex modes are named CSImode, etc.
This inconsistency should be eliminated. */
if (cl == MODE_FLOAT)
{
char *p, *q = 0;
- strncpy (buf, m-name, sizeof buf);
+ strcpy (buf, m-name);
p = strchr (buf, 'F');
if (p == 0)
q = strchr (buf, 'D');
if (p == 0 q == 0)
{
error (%s:%d: float mode \%s\ has no 'F' or 'D',
m-file, m-line, m-name);
continue;
}
if (p != 0)
*p = 'C';
else
--
1.7.10.208.gb4267
Re: [PATCH] genmodes: remove misleading use of strncpy
On Thu, Apr 19, 2012 at 02:01:36PM +0200, Richard Guenther wrote: strncpy - strcpy definitely looks like premature optimization to me. I disagree. Almost all uses of strncpy are wrong. If the string length is smaller, than it is unlikely useful that it clears all remaining bytes (sometimes many of them) - exception might be security sensitive code, if the string length is known to be sizeof buf - 1, then strcpy or even memcpy should be used, and if the string length is larger, then no '\0' is appended, so it is unlikely the right thing. Yes, Jim should probably add a comment. Jakub
Re: [PATCH] genmodes: remove misleading use of strncpy
Richard Guenther wrote:
On Thu, Apr 19, 2012 at 12:42 PM, Jim Meyering j...@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)
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.
IMHO, if you'd like to avoid the duplication in the strcpy/snprintf
path, that deserves to be a different change.
Re: [PATCH] genmodes: remove misleading use of strncpy
On Thu, Apr 19, 2012 at 2:13 PM, Jim Meyering j...@meyering.net wrote:
Richard Guenther wrote:
On Thu, Apr 19, 2012 at 12:42 PM, Jim Meyering j...@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.
Re: [PATCH] genmodes: remove misleading use of strncpy
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
Re: [PATCH] genmodes: remove misleading use of strncpy
On Thu, Apr 19, 2012 at 2:34 PM, Jakub Jelinek ja...@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
Re: [PATCH] genmodes: remove misleading use of strncpy
On Thu, Apr 19, 2012 at 02:38:11PM +0200, Richard Guenther wrote: On Thu, Apr 19, 2012 at 2:34 PM, Jakub Jelinek ja...@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
Re: [PATCH] genmodes: remove misleading use of strncpy
Richard Guenther wrote:
On Thu, Apr 19, 2012 at 2:13 PM, Jim Meyering j...@meyering.net wrote:
Richard Guenther wrote:
On Thu, Apr 19, 2012 at 12:42 PM, Jim Meyering j...@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 meyer...@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
Re: [PATCH] genmodes: remove misleading use of strncpy
On Thu, Apr 19, 2012 at 3:01 PM, Jim Meyering j...@meyering.net wrote:
Richard Guenther wrote:
On Thu, Apr 19, 2012 at 2:13 PM, Jim Meyering j...@meyering.net wrote:
Richard Guenther wrote:
On Thu, Apr 19, 2012 at 12:42 PM, Jim Meyering j...@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 meyer...@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
Re: [PATCH] genmodes: remove misleading use of strncpy
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.
