From: Julia Lawall <[email protected]>
As observed by Joe Perches, sizeof of a constant string includes the
trailing 0. If what is wanted is to check the initial characters of
another string, this trailing 0 should not be taken into account. If an
exact match is wanted, strcmp should be used instead.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression foo;
constant char *abc;
@@
strncmp(foo, abc,
- sizeof(abc)
+ sizeof(abc)-1
)
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
fs/qnx4/inode.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index 449f5a6..0614b00 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -221,7 +221,9 @@ static const char *qnx4_checkroot(struct super_block *sb)
rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
if (rootdir->di_fname != NULL) {
QNX4DEBUG((KERN_INFO "rootdir entry found : [%s]\n", rootdir->di_fname));
- if (!strncmp(rootdir->di_fname, QNX4_BMNAME, sizeof QNX4_BMNAME)) {
+ if (!strncmp(rootdir->di_fname,
+ QNX4_BMNAME,
+ sizeof QNX4_BMNAME - 1)) {
found = 1;
qnx4_sb(sb)->BitMap = kmalloc( sizeof( struct qnx4_inode_entry ), GFP_KERNEL );
if (!qnx4_sb(sb)->BitMap) {
On 2009-11-12 08:49:44, Julia Lawall wrote:
> As observed by Joe Perches, sizeof of a constant string includes the
> trailing 0. If what is wanted is to check the initial characters of
> another string, this trailing 0 should not be taken into account. If an
> exact match is wanted, strcmp should be used instead.
Good catch.
However, in this case an exact match (with the string ".bitmap") is what
is needed, so
NAK
Will you cook up a new patch changing strncmp to strcmp?
Cheers
Anders
On Thu, 12 Nov 2009, Anders Larsen wrote:
> On 2009-11-12 08:49:44, Julia Lawall wrote:
> > As observed by Joe Perches, sizeof of a constant string includes the
> > trailing 0. If what is wanted is to check the initial characters of
> > another string, this trailing 0 should not be taken into account. If an
> > exact match is wanted, strcmp should be used instead.
>
> Good catch.
> However, in this case an exact match (with the string ".bitmap") is what
> is needed, so
>
> NAK
>
> Will you cook up a new patch changing strncmp to strcmp?
Sure.
julia
From: Julia Lawall <[email protected]>
As an identical match is wanted in this case, strcmp can be used instead.
The semantic match that lead to detecting this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression foo;
constant char *abc;
@@
*strncmp(foo, abc, sizeof(abc))
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
fs/qnx4/inode.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index 449f5a6..150f4af 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -221,7 +221,8 @@ static const char *qnx4_checkroot(struct super_block *sb)
rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
if (rootdir->di_fname != NULL) {
QNX4DEBUG((KERN_INFO "rootdir entry found : [%s]\n", rootdir->di_fname));
- if (!strncmp(rootdir->di_fname, QNX4_BMNAME, sizeof QNX4_BMNAME)) {
+ if (!strcmp(rootdir->di_fname,
+ QNX4_BMNAME)) {
found = 1;
qnx4_sb(sb)->BitMap = kmalloc( sizeof( struct qnx4_inode_entry ), GFP_KERNEL );
if (!qnx4_sb(sb)->BitMap) {
On Thu, 2009-11-12 at 08:49 +0100, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> As observed by Joe Perches, sizeof of a constant string includes the
> trailing 0. If what is wanted is to check the initial characters of
> another string, this trailing 0 should not be taken into account. If an
> exact match is wanted, strcmp should be used instead.
[...]
> strncmp(foo, abc,
> - sizeof(abc)
> + sizeof(abc)-1
> )
> // </smpl>
Am I the only one who find "strlen()" instead of "sizeof()-1" here much
more readable (and to the point).
As for run-time, it shouldn't make a difference for static/constant
strings as gcc should be able calculate the length at compile time. And
if the string is not constant, sizeof() is probably wrong anyways.
Bernd
--
Firmix Software GmbH http://www.firmix.at/
mobil: +43 664 4416156 fax: +43 1 7890849-55
Embedded Linux Development and Services
On Thu, 12 Nov 2009, Bernd Petrovitsch wrote:
> On Thu, 2009-11-12 at 08:49 +0100, Julia Lawall wrote:
> > From: Julia Lawall <[email protected]>
> >
> > As observed by Joe Perches, sizeof of a constant string includes the
> > trailing 0. If what is wanted is to check the initial characters of
> > another string, this trailing 0 should not be taken into account. If an
> > exact match is wanted, strcmp should be used instead.
> [...]
> > strncmp(foo, abc,
> > - sizeof(abc)
> > + sizeof(abc)-1
> > )
> > // </smpl>
> Am I the only one who find "strlen()" instead of "sizeof()-1" here much
> more readable (and to the point).
>
> As for run-time, it shouldn't make a difference for static/constant
> strings as gcc should be able calculate the length at compile time. And
> if the string is not constant, sizeof() is probably wrong anyways.
The rule specifies that the string is a constant.
julia
On Thu, 12 Nov 2009, Bernd Petrovitsch wrote:
> On Thu, 2009-11-12 at 08:49 +0100, Julia Lawall wrote:
> > From: Julia Lawall <[email protected]>
> >
> > As observed by Joe Perches, sizeof of a constant string includes the
> > trailing 0. If what is wanted is to check the initial characters of
> > another string, this trailing 0 should not be taken into account. If an
> > exact match is wanted, strcmp should be used instead.
> [...]
> > strncmp(foo, abc,
> > - sizeof(abc)
> > + sizeof(abc)-1
> > )
> > // </smpl>
> Am I the only one who find "strlen()" instead of "sizeof()-1" here much
> more readable (and to the point).
>
> As for run-time, it shouldn't make a difference for static/constant
> strings as gcc should be able calculate the length at compile time. And
> if the string is not constant, sizeof() is probably wrong anyways.
Does gcc have access to the definition of strlen? It does not seem to be
an inlined function, eg in lib/string.c.
julia
From: Julia Lawall <[email protected]>
As an identical match is wanted in this case, strcmp can be used instead.
The semantic match that lead to detecting this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression foo;
constant char *abc;
@@
*strncmp(foo, abc, sizeof(abc))
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
Signed-off-by: Anders Larsen <[email protected]>
---
fs/qnx4/inode.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index 449f5a6..150f4af 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -221,7 +221,8 @@ static const char *qnx4_checkroot(struct super_block *sb)
rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
if (rootdir->di_fname != NULL) {
QNX4DEBUG((KERN_INFO "rootdir entry found : [%s]\n", rootdir->di_fname));
- if (!strncmp(rootdir->di_fname, QNX4_BMNAME, sizeof QNX4_BMNAME)) {
+ if (!strcmp(rootdir->di_fname,
+ QNX4_BMNAME)) {
found = 1;
qnx4_sb(sb)->BitMap = kmalloc( sizeof( struct qnx4_inode_entry ), GFP_KERNEL );
if (!qnx4_sb(sb)->BitMap) {
Julia Lawall wrote:
> As observed by Joe Perches, sizeof of a constant string includes the
> trailing 0. If what is wanted is to check the initial characters of
> another string, this trailing 0 should not be taken into account. If an
> exact match is wanted, strcmp should be used instead.
>
> The semantic patch that makes this change is as follows:
A caution: Your patch changes behavior. Is there a specific reason
to believe that the change in behavior is what is desired/intended in
this context? Lacking any analysis that indicates that the change in
behavior is desired, I'm skeptical that a behavior-changing patch should
be applied.
On Thu, 2009-11-12 at 23:49 +0000, David Wagner wrote:
> Julia Lawall wrote:
> > As observed by Joe Perches, sizeof of a constant string includes the
> > trailing 0. If what is wanted is to check the initial characters of
> > another string, this trailing 0 should not be taken into account. If an
> > exact match is wanted, strcmp should be used instead.
> A caution: Your patch changes behavior. Is there a specific reason
> to believe that the change in behavior is what is desired/intended in
> this context? Lacking any analysis that indicates that the change in
> behavior is desired, I'm skeptical that a behavior-changing patch should
> be applied.
I agree that desired behavior should be known before
patches are applied.
Most likely, this instance should just be strcmp.
That gives better readability, trivially improved
performance, and easier verification.
On Thu, 2009-11-12 at 16:33 +0100, Julia Lawall wrote:
> On Thu, 12 Nov 2009, Bernd Petrovitsch wrote:
>
> > On Thu, 2009-11-12 at 08:49 +0100, Julia Lawall wrote:
> > > From: Julia Lawall <[email protected]>
> > >
> > > As observed by Joe Perches, sizeof of a constant string includes the
> > > trailing 0. If what is wanted is to check the initial characters of
> > > another string, this trailing 0 should not be taken into account. If an
> > > exact match is wanted, strcmp should be used instead.
> > [...]
> > > strncmp(foo, abc,
> > > - sizeof(abc)
> > > + sizeof(abc)-1
> > > )
> > > // </smpl>
> > Am I the only one who find "strlen()" instead of "sizeof()-1" here much
> > more readable (and to the point).
> >
> > As for run-time, it shouldn't make a difference for static/constant
> > strings as gcc should be able calculate the length at compile time. And
> > if the string is not constant, sizeof() is probably wrong anyways.
>
> Does gcc have access to the definition of strlen? It does not seem to be
> an inlined function, eg in lib/string.c.
Since "strlen()" is defined in the C-Standard C-compilers could rely on
the defined behaviour (but I don't know exactly how gcc behaves with
-ffreestanding for all supported versions).
Then there is __builin_strlen() (see also
http://gcc.gnu.org/onlinedocs/gcc-4.4.2/gcc/Other-Builtins.html#Other-Builtins).
Stepping a quite small abstraction layer higher:
include/linux/string.h has at teh end:
---- snip ----
static inline bool strstarts(const char *str, const char *prefix)
{
return strncmp(str, prefix, strlen(prefix)) == 0;
}
---- snip ----
seems to be what most uses of strnmcp() actually are: check if one
string is a prefix of another.
Bernd
--
Firmix Software GmbH http://www.firmix.at/
mobil: +43 664 4416156 fax: +43 1 7890849-55
Embedded Linux Development and Services
On Thu, 12 Nov 2009, Anders Larsen wrote:
> From: Julia Lawall <[email protected]>
>
> As an identical match is wanted in this case, strcmp can be used instead.
>
> The semantic match that lead to detecting this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression foo;
> constant char *abc;
> @@
>
> *strncmp(foo, abc, sizeof(abc))
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
> Signed-off-by: Anders Larsen <[email protected]>
>
> ---
> fs/qnx4/inode.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
> index 449f5a6..150f4af 100644
> --- a/fs/qnx4/inode.c
> +++ b/fs/qnx4/inode.c
> @@ -221,7 +221,8 @@ static const char *qnx4_checkroot(struct super_block *sb)
> rootdir = (struct qnx4_inode_entry *) (bh->b_data + i * QNX4_DIR_ENTRY_SIZE);
> if (rootdir->di_fname != NULL) {
> QNX4DEBUG((KERN_INFO "rootdir entry found : [%s]\n", rootdir->di_fname));
> - if (!strncmp(rootdir->di_fname, QNX4_BMNAME, sizeof QNX4_BMNAME)) {
> + if (!strcmp(rootdir->di_fname,
> + QNX4_BMNAME)) {
> found = 1;
> qnx4_sb(sb)->BitMap = kmalloc( sizeof( struct qnx4_inode_entry ), GFP_KERNEL );
> if (!qnx4_sb(sb)->BitMap) {
Doesn't seem to be present in 2.6.33-rc1.
Applied to my queue, thanks.
--
Jiri Kosina
SUSE Labs, Novell Inc.