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

[Eric Sesterhenn / Snakebyte <snakebyte@gmx.de>]

* 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