[Patch] Off by one in drivers/usb/input/yealink.c

[Patch] Off by one in drivers/usb/input/yealink.c

[Eric Sesterhenn]

hi,

another off by one spotted by coverity (id #485),
we loop exactly one time too often

Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>

--- linux-2.6.17-git11/drivers/usb/input/yealink.c.orig	2006-06-28 00:29:46.000000000 +0200
+++ linux-2.6.17-git11/drivers/usb/input/yealink.c	2006-06-28 00:30:04.000000000 +0200
@@ -350,7 +350,7 @@ static int yealink_do_idle_tasks(struct 
 		val = yld->master.b[ix];
 		if (val != yld->copy.b[ix])
 			goto send_update;
-	} while (++ix < sizeof(yld->master));
+	} while (++ix < sizeof(yld->master)-1);
 
 	/* nothing todo, wait a bit and poll for a KEYPRESS */
 	yld->stat_ix = 0;

Re: [Patch] Off by one in drivers/usb/input/yealink.c

[Randy.Dunlap]

On Wed, 28 Jun 2006 00:41:19 +0200 Eric Sesterhenn wrote:

> hi,
> 
> another off by one spotted by coverity (id #485),
> we loop exactly one time too often
> 
> Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
> 
> --- linux-2.6.17-git11/drivers/usb/input/yealink.c.orig	2006-06-28 00:29:46.000000000 +0200
> +++ linux-2.6.17-git11/drivers/usb/input/yealink.c	2006-06-28 00:30:04.000000000 +0200
> @@ -350,7 +350,7 @@ static int yealink_do_idle_tasks(struct 
>  		val = yld->master.b[ix];
>  		if (val != yld->copy.b[ix])
>  			goto send_update;
> -	} while (++ix < sizeof(yld->master));
> +	} while (++ix < sizeof(yld->master)-1);
>  
>  	/* nothing todo, wait a bit and poll for a KEYPRESS */
>  	yld->stat_ix = 0;

FWIW, on this one and the previous floppy.c patch,
I would rather see the comparison be <= instead of using -1.

---
~Randy

Re: [Patch] Off by one in drivers/usb/input/yealink.c

[Eric Sesterhenn / Snakebyte]

* Randy.Dunlap (rdunlap@xenotime.net) wrote:
> On Wed, 28 Jun 2006 00:41:19 +0200 Eric Sesterhenn wrote:
> 
> > hi,
> > 
> > another off by one spotted by coverity (id #485),
> > we loop exactly one time too often
> > 
> > Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
> > 
> > --- linux-2.6.17-git11/drivers/usb/input/yealink.c.orig	2006-06-28 00:29:46.000000000 +0200
> > +++ linux-2.6.17-git11/drivers/usb/input/yealink.c	2006-06-28 00:30:04.000000000 +0200
> > @@ -350,7 +350,7 @@ static int yealink_do_idle_tasks(struct 
> >  		val = yld->master.b[ix];
> >  		if (val != yld->copy.b[ix])
> >  			goto send_update;
> > -	} while (++ix < sizeof(yld->master));
> > +	} while (++ix < sizeof(yld->master)-1);
> >  
> >  	/* nothing todo, wait a bit and poll for a KEYPRESS */
> >  	yld->stat_ix = 0;
> 
> FWIW, on this one and the previous floppy.c patch,
> I would rather see the comparison be <= instead of using -1.

maybe it is too late, but wouldnt the <= make the loop
run even more iterations, like (++ix < sizeof() + 1)

greetings, Eric

Re: [Patch] Off by one in drivers/usb/input/yealink.c

[Randy.Dunlap]

On Wed, 28 Jun 2006 01:04:16 +0200 Eric Sesterhenn / Snakebyte wrote:

> * Randy.Dunlap (rdunlap@xenotime.net) wrote:
> > On Wed, 28 Jun 2006 00:41:19 +0200 Eric Sesterhenn wrote:
> > 
> > > another off by one spotted by coverity (id #485),
> > > we loop exactly one time too often
> > > 
> > > --- linux-2.6.17-git11/drivers/usb/input/yealink.c.orig	2006-06-28 00:29:46.000000000 +0200
> > > +++ linux-2.6.17-git11/drivers/usb/input/yealink.c	2006-06-28 00:30:04.000000000 +0200
> > > @@ -350,7 +350,7 @@ static int yealink_do_idle_tasks(struct 
> > >  		val = yld->master.b[ix];
> > >  		if (val != yld->copy.b[ix])
> > >  			goto send_update;
> > > -	} while (++ix < sizeof(yld->master));
> > > +	} while (++ix < sizeof(yld->master)-1);
> > >  
> > >  	/* nothing todo, wait a bit and poll for a KEYPRESS */
> > >  	yld->stat_ix = 0;
> > 
> > FWIW, on this one and the previous floppy.c patch,
> > I would rather see the comparison be <= instead of using -1.
> 
> maybe it is too late, but wouldnt the <= make the loop
> run even more iterations, like (++ix < sizeof() + 1)

:) Yep.

so for the floppy.c patch, I still prefer to see:
+	if (drive < 0 || drive >= N_DRIVE) {

instead of
+	if (drive < 0 || drive > N_DRIVE-1) {

Does that make sense?

---
~Randy

Re: [Patch] Off by one in drivers/usb/input/yealink.c

[Eric Sesterhenn]

> :) Yep.
> 
> so for the floppy.c patch, I still prefer to see:
> +	if (drive < 0 || drive >= N_DRIVE) {
> 
> instead of
> +	if (drive < 0 || drive > N_DRIVE-1) {
> 
> Does that make sense?

looks better :)

--- 

another bug spotted by coverity (id #481).
In the case that drive == N_DRIVE we acess past the
drive_params array which is defined as 
drive_params[N_DRIVE]. By using the UDP define
in the else case because UDP is &drive_params[drive]

Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>

--- linux-2.6.17-git11/drivers/block/floppy.c.orig	2006-06-28 01:22:59.000000000 +0200
+++ linux-2.6.17-git11/drivers/block/floppy.c	2006-06-28 01:23:24.000000000 +0200
@@ -684,7 +684,7 @@ static void __reschedule_timeout(int dri
 	if (drive == current_reqD)
 		drive = current_drive;
 	del_timer(&fd_timeout);
-	if (drive < 0 || drive > N_DRIVE) {
+	if (drive < 0 || drive >= N_DRIVE) {
 		fd_timeout.expires = jiffies + 20UL * HZ;
 		drive = 0;
 	} else

Re: [Patch] Off by one in drivers/usb/input/yealink.c

[Henk Vergonet]

Sorry for the late response guys, but I was on holliday.

No no no, please dont use this.


On Tue, Jun 27, 2006 at 03:51:43PM -0700, Randy.Dunlap wrote:
> On Wed, 28 Jun 2006 00:41:19 +0200 Eric Sesterhenn wrote:
> > another off by one spotted by coverity (id #485),
> > we loop exactly one time too often
> > 
> > Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
> > 
> > --- linux-2.6.17-git11/drivers/usb/input/yealink.c.orig	2006-06-28 00:29:46.000000000 +0200
> > +++ linux-2.6.17-git11/drivers/usb/input/yealink.c	2006-06-28 00:30:04.000000000 +0200
> > @@ -350,7 +350,7 @@ static int yealink_do_idle_tasks(struct 
> >  		val = yld->master.b[ix];
> >  		if (val != yld->copy.b[ix])
> >  			goto send_update;
> > -	} while (++ix < sizeof(yld->master));
> > +	} while (++ix < sizeof(yld->master)-1);

Apart from introducing a new bug in the code, the construct is ugly.

I would rather see then the more readable:

	ix++;
    } while (ix < sizeof(yld->master));
 
- Henk

Re: [Patch] Off by one in drivers/usb/input/yealink.c

[Eric Sesterhenn / Snakebyte]

* Dmitry Torokhov (dmitry.torokhov@gmail.com) wrote:
> On 7/5/06, Henk Vergonet <Henk.Vergonet@gmail.com> wrote:
> >On Tue, Jun 27, 2006 at 03:51:43PM -0700, Randy.Dunlap wrote:
> >> On Wed, 28 Jun 2006 00:41:19 +0200 Eric Sesterhenn wrote:
> >> > another off by one spotted by coverity (id #485),
> >> > we loop exactly one time too often
> >> >
> >> > Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
> >> >
> >> > --- linux-2.6.17-git11/drivers/usb/input/yealink.c.orig     2006-06-28 
> >00:29:46.000000000 +0200
> >> > +++ linux-2.6.17-git11/drivers/usb/input/yealink.c  2006-06-28 
> >00:30:04.000000000 +0200
> >> > @@ -350,7 +350,7 @@ static int yealink_do_idle_tasks(struct
> >> >             val = yld->master.b[ix];
> >> >             if (val != yld->copy.b[ix])
> >> >                     goto send_update;
> >> > -   } while (++ix < sizeof(yld->master));
> >> > +   } while (++ix < sizeof(yld->master)-1);
> >
> >Apart from introducing a new bug in the code, the construct is ugly.
> >
> >I would rather see then the more readable:
> >
> >       ix++;
> >   } while (ix < sizeof(yld->master));
> >
> 
> The new code is exactly the same as the old one; however I do not see
> the problem with the old code. Could it be that Coverity got confused
> by prefix vs. postfix increment?

I looked at this code several times too, and tried to reproduce the bug
with the following little program:

#include <string.h>
int main(int argc, char **argv) {
	char foo[] = "abcdef";
	int i = 0;

	foo[strlen(foo)] = 'X';
	do {
		putchar(foo[i]);
	} while (++i < sizeof(foo));
}

Which clearly shows that the terminating '\0' gets printed too,
replaced by the X for better visibility, so the code
runs past the array, or did I fail to replicate the original
code somewhere?

Eric

Re: [Patch] Off by one in drivers/usb/input/yealink.c

[Dmitry Torokhov]

On Wednesday 05 July 2006 20:49, Eric Sesterhenn / Snakebyte wrote:
> * Dmitry Torokhov (dmitry.torokhov@gmail.com) wrote:
> > On 7/5/06, Henk Vergonet <Henk.Vergonet@gmail.com> wrote:
> > >On Tue, Jun 27, 2006 at 03:51:43PM -0700, Randy.Dunlap wrote:
> > >> On Wed, 28 Jun 2006 00:41:19 +0200 Eric Sesterhenn wrote:
> > >> > another off by one spotted by coverity (id #485),
> > >> > we loop exactly one time too often
> > >> >
> > >> > Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
> > >> >
> > >> > --- linux-2.6.17-git11/drivers/usb/input/yealink.c.orig     2006-06-28 
> > >00:29:46.000000000 +0200
> > >> > +++ linux-2.6.17-git11/drivers/usb/input/yealink.c  2006-06-28 
> > >00:30:04.000000000 +0200
> > >> > @@ -350,7 +350,7 @@ static int yealink_do_idle_tasks(struct
> > >> >             val = yld->master.b[ix];
> > >> >             if (val != yld->copy.b[ix])
> > >> >                     goto send_update;
> > >> > -   } while (++ix < sizeof(yld->master));
> > >> > +   } while (++ix < sizeof(yld->master)-1);
> > >
> > >Apart from introducing a new bug in the code, the construct is ugly.
> > >
> > >I would rather see then the more readable:
> > >
> > >       ix++;
> > >   } while (ix < sizeof(yld->master));
> > >
> > 
> > The new code is exactly the same as the old one; however I do not see
> > the problem with the old code. Could it be that Coverity got confused
> > by prefix vs. postfix increment?
> 
> I looked at this code several times too, and tried to reproduce the bug
> with the following little program:
> 
> #include <string.h>
> int main(int argc, char **argv) {
> 	char foo[] = "abcdef";
> 	int i = 0;
> 
> 	foo[strlen(foo)] = 'X';
> 	do {
> 		putchar(foo[i]);
> 	} while (++i < sizeof(foo));
> }
> 
> Which clearly shows that the terminating '\0' gets printed too,
> replaced by the X for better visibility, so the code
> runs past the array, or did I fail to replicate the original
> code somewhere?
> 

What do you mean "the code runs past the array"? The size of array is 7
(compiler allocates the space for terminating '\0') and the array is
printed in its entirety.

-- 
Dmitry

Re: [Patch] Off by one in drivers/usb/input/yealink.c

[Eric Sesterhenn / Snakebyte]

* Dmitry Torokhov (dtor@insightbb.com) wrote:
> On Wednesday 05 July 2006 20:49, Eric Sesterhenn / Snakebyte wrote:
> > I looked at this code several times too, and tried to reproduce the bug
> > with the following little program:
> > 
> > #include <string.h>
> > int main(int argc, char **argv) {
> > 	char foo[] = "abcdef";
> > 	int i = 0;
> > 
> > 	foo[strlen(foo)] = 'X';
> > 	do {
> > 		putchar(foo[i]);
> > 	} while (++i < sizeof(foo));
> > }
> > 
> > Which clearly shows that the terminating '\0' gets printed too,
> > replaced by the X for better visibility, so the code
> > runs past the array, or did I fail to replicate the original
> > code somewhere?
> > 
> 
> What do you mean "the code runs past the array"? The size of array is 7
> (compiler allocates the space for terminating '\0') and the array is
> printed in its entirety.

arg, of course \0 is part of the array, sorry bothering you guys
with this stuff :( Another coverity report i analyzed incorrect... :(

Eric