Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp

Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp

[Anders Larsen]

From: Julia Lawall <julia@diku.dk>

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 <julia@diku.dk>
Signed-off-by: Anders Larsen <al@alarsen.net>

---
 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) {

Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp

[Jiri Kosina]

On Thu, 12 Nov 2009, Anders Larsen wrote:

> From: Julia Lawall <julia@diku.dk>
> 
> 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 <julia@diku.dk>
> Signed-off-by: Anders Larsen <al@alarsen.net>
> 
> ---
>  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.

Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp

[Joe Perches]

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.

Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp

[David Wagner]

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.

Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

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 <julia@diku.dk>

---
 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) {

Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp

[Julia Lawall]

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

Re: [PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp

[Anders Larsen]

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

[PATCH 4/4] fs/qnx4: decrement sizeof size in strncmp

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

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 <julia@diku.dk>

---
 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) {