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@xxxxxxxxx) wrote:
> > On 7/5/06, Henk Vergonet <Henk.Vergonet@xxxxxxxxx> 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@xxxxxx>
> > >> >
> > >> > --- 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/