Re: Coverity fix for games/hunt

[Jasper Lievisse Adriaanse <jasper_(_at_)_nedbsd_(_dot_)_nl>]

On Wed, 22 Mar 2006 16:30:35 -0500
Ray Lai <ray_(_at_)_cyth_(_dot_)_net> wrote:

> On Wed, Mar 22, 2006 at 10:09:07PM +0100, Jasper Lievisse Adriaanse wrote:
> > On Wed, 22 Mar 2006 14:02:11 -0500
> > Ray Lai <ray_(_at_)_cyth_(_dot_)_net> wrote:
> >
> > > On Wed, Mar 22, 2006 at 07:15:50PM +0100, Jasper Lievisse Adriaanse
wrote:
> > > > ---------------------8<---------------------
> > > > Coverity CID 1443, from christos_(_at_)_netbsd_(_dot_)_org
> > >
> > > Instead of just specifying the Coverity CID, can you provide
> > > justifications for these patches?  Just because Coverity says it's
> > > a bug doesn't mean it's a bug, and just because NetBSD fixed it
> > > this way doesn't mean it's correct.
> > Like specifying: ''Coverity CID 1443: Prevent static overrun.'' or be
more
> > precise?
> >
> > Thanks for at least looking at them ;-)
> >
> > > > Index: hunt/otto.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/games/hunt/hunt/otto.c,v
> > > > retrieving revision 1.8
> > > > diff -u -r1.8 otto.c
> > > > --- hunt/otto.c	7 Aug 2003 20:19:10 -0000	1.8
> > > > +++ hunt/otto.c	22 Mar 2006 17:51:21 -0000
> > > > @@ -294,7 +294,8 @@
> > > >  	cont_north:
> > > >  		if (itemp->flags & DEADEND) {
> > > >  			itemp->flags |= BEEN;
> > > > -			been_there[r][col] |= NORTH;
> > > > +			if (r >= 0)
> > > > +				been_there[r][col] |= NORTH;
> > > >  			for (r = row - 1; r > row - itemp->distance; r--)
> > > >  				been_there[r][col] = ALLDIRS;
> > > >  		}
>
> Like here.  Why are we checking if r >= 0?  I don't see where it's
> being set so I don't know if it's signed, unsigned, part of a loop,
> the result of a system call... there's no information.
``r'' is being set to ``0'' in line 276. After "case NORTH" there's a
for-loop: ``for (r = row - 1; r >= 0; r--)''. So ``r'' is being manipulated
here until it is greater than, or it equals ``0''.

Here are the statements to check whether the code above (in the diff)
should be executed:
if (stop_look(itemp, ch, row - r, c - col))
	goto cont_north;
>
> > > > @@ -314,7 +315,8 @@
> > > >  	cont_south:
> > > >  		if (itemp->flags & DEADEND) {
> > > >  			itemp->flags |= BEEN;
> > > > -			been_there[r][col] |= SOUTH;
> > > > +			if (r < HEIGHT)
> > > > +				been_there[r][col] |= SOUTH;
> > > >  			for (r = row + 1; r < row + itemp->distance; r++)
> > > >  				been_there[r][col] = ALLDIRS;
> > > >  		}
>
> Ditto about this.  Why does r have to be less than HEIGHT?

126:	static	char been_there[HEIGHT][WIDTH2];
158:	row = y; col = x;
159:	been_there[row][col] |= 1 << facing;

so, if ``r'' is less then the current height, we're moving south.

>
> It would be nice if there were code excerpts interleaved with
> commentary to explain why this is wrong, followed by a diff.
>
> Yes, I could go through it myself and figure it out.  But if you
> know what's wrong with it, why not share this information and speed
> up the process?  And if you don't know what's wrong with it, well,
> you shouldn't be submitting this diff.
>
> You replied to me privately about this, but I would like to have
> this posted on tech@ or something, for others who are submitting
> diffs.  If you don't mind, please reply or forward this to the list
> as well.
>
> -Ray-

Cheers,
Jasper


--
Humppa is a serious thing!

[demime 1.01d removed an attachment of type application/pgp-signature]