[RFC: 2.6 patch] drivers/net/wireless/airo.c: correct a wrong

[RFC: 2.6 patch] drivers/net/wireless/airo.c: correct a wrong

[Adrian Bunk]

if
Reply-To: 

The Coverity checker correctly noted that this condition can't ever be 
fulfilled.

Can someone understanding this code check whether my guess what this 
should have been was right?

Or should the if get completely dropped?

Signed-off-by: Adrian Bunk <bunk@stusta.de>

--- linux-2.6.12-rc1-mm1-full/drivers/net/wireless/airo.c.old	2005-03-22 21:41:37.000000000 +0100
+++ linux-2.6.12-rc1-mm1-full/drivers/net/wireless/airo.c	2005-03-22 21:42:01.000000000 +0100
@@ -3440,9 +3440,6 @@
 	/* Make sure we got something */
 	if (rxd.rdy && rxd.valid == 0) {
 		len = rxd.len + 12;
-		if (len < 12 && len > 2048)
-			goto badrx;
-
 		skb = dev_alloc_skb(len);
 		if (!skb) {
 			ai->stats.rx_dropped++;

Re: [RFC: 2.6 patch] drivers/net/wireless/airo.c: correct a wrong

[Jeff Garzik]

Adrian Bunk wrote:
> if
> Reply-To: 
> 
> The Coverity checker correctly noted that this condition can't ever be 
> fulfilled.
> 
> Can someone understanding this code check whether my guess what this 
> should have been was right?
> 
> Or should the if get completely dropped?
> 
> Signed-off-by: Adrian Bunk <bunk@stusta.de>
> 
> --- linux-2.6.12-rc1-mm1-full/drivers/net/wireless/airo.c.old	2005-03-22 21:41:37.000000000 +0100
> +++ linux-2.6.12-rc1-mm1-full/drivers/net/wireless/airo.c	2005-03-22 21:42:01.000000000 +0100
> @@ -3440,9 +3440,6 @@
>  	/* Make sure we got something */
>  	if (rxd.rdy && rxd.valid == 0) {
>  		len = rxd.len + 12;
> -		if (len < 12 && len > 2048)
> -			goto badrx;

Coverity is silly.

len is signed, and so can obviously be less than zero in edge cases.  I 
don't see where the "> 2048" test is invalid, either.

	Jeff

Re: [RFC: 2.6 patch] drivers/net/wireless/airo.c: correct a wrong

[Adrian Bunk]

On Tue, Mar 22, 2005 at 05:17:21PM -0500, Jeff Garzik wrote:
> Adrian Bunk wrote:
> >if
> >Reply-To: 
> >
> >The Coverity checker correctly noted that this condition can't ever be 
> >fulfilled.
> >
> >Can someone understanding this code check whether my guess what this 
> >should have been was right?
> >
> >Or should the if get completely dropped?
> >
> >Signed-off-by: Adrian Bunk <bunk@stusta.de>
> >
> >--- linux-2.6.12-rc1-mm1-full/drivers/net/wireless/airo.c.old	2005-03-22 
> >21:41:37.000000000 +0100
> >+++ linux-2.6.12-rc1-mm1-full/drivers/net/wireless/airo.c	2005-03-22 
> >21:42:01.000000000 +0100
> >@@ -3440,9 +3440,6 @@
> > 	/* Make sure we got something */
> > 	if (rxd.rdy && rxd.valid == 0) {
> > 		len = rxd.len + 12;
> >-		if (len < 12 && len > 2048)
> >-			goto badrx;
> 
> Coverity is silly.
> 
> len is signed, and so can obviously be less than zero in edge cases.  I 
> don't see where the "> 2048" test is invalid, either.

But if it's less than zero it can't be > 2048 at the same time?

The point is: len can't be both < 12 and > 2048 at the same time.


Is this "if" simply superfluous?
Or should the && be an || ?


> 	Jeff

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [RFC: 2.6 patch] drivers/net/wireless/airo.c: correct a wrong

[Jeff Garzik]

On Tue, Mar 22, 2005 at 11:30:56PM +0100, Adrian Bunk wrote:
> On Tue, Mar 22, 2005 at 05:17:21PM -0500, Jeff Garzik wrote:
> > Adrian Bunk wrote:
> > >if
> > >Reply-To: 
> > >
> > >The Coverity checker correctly noted that this condition can't ever be 
> > >fulfilled.
> > >
> > >Can someone understanding this code check whether my guess what this 
> > >should have been was right?
> > >
> > >Or should the if get completely dropped?
> > >
> > >Signed-off-by: Adrian Bunk <bunk@stusta.de>
> > >
> > >--- linux-2.6.12-rc1-mm1-full/drivers/net/wireless/airo.c.old	2005-03-22 
> > >21:41:37.000000000 +0100
> > >+++ linux-2.6.12-rc1-mm1-full/drivers/net/wireless/airo.c	2005-03-22 
> > >21:42:01.000000000 +0100
> > >@@ -3440,9 +3440,6 @@
> > > 	/* Make sure we got something */
> > > 	if (rxd.rdy && rxd.valid == 0) {
> > > 		len = rxd.len + 12;
> > >-		if (len < 12 && len > 2048)
> > >-			goto badrx;
> > 
> > Coverity is silly.
> > 
> > len is signed, and so can obviously be less than zero in edge cases.  I 
> > don't see where the "> 2048" test is invalid, either.
> 
> But if it's less than zero it can't be > 2048 at the same time?
> 
> The point is: len can't be both < 12 and > 2048 at the same time.
> 
> 
> Is this "if" simply superfluous?
> Or should the && be an || ?

Yes, it looks like it should be "||".

	Jeff

[2.6 patch] drivers/net/wireless/airo.c: correct a wrong check

[Adrian Bunk]

On Tue, Mar 22, 2005 at 05:34:03PM -0500, Jeff Garzik wrote:
> On Tue, Mar 22, 2005 at 11:30:56PM +0100, Adrian Bunk wrote:
>...
> > Is this "if" simply superfluous?
> > Or should the && be an || ?
> 
> Yes, it looks like it should be "||".

Patch below.

> 	Jeff

cu
Adrian


<--  snip  -->


The Coverity checker correctly noted that this condition can't ever be 
fulfilled.

This patch changes it to what it should have been.

Signed-off-by: Adrian Bunk <bunk@stusta.de>

--- linux-2.6.12-rc1-mm1-full/drivers/net/wireless/airo.c.old	2005-03-22 21:41:37.000000000 +0100
+++ linux-2.6.12-rc1-mm1-full/drivers/net/wireless/airo.c	2005-03-22 22:50:03.000000000 +0100
@@ -3440,7 +3440,7 @@
 	/* Make sure we got something */
 	if (rxd.rdy && rxd.valid == 0) {
 		len = rxd.len + 12;
-		if (len < 12 && len > 2048)
+		if (len < 12 || len > 2048)
 			goto badrx;
 
 		skb = dev_alloc_skb(len);