[Freeciv-Dev] Re: [PATCH] bug in vnotify_conn_ex()

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:

> On Wed, Jan 02, 2002 at 03:44:18AM -0500, Jason Short wrote:
>
> > This buglet is very similar to PR#1184.
> >
> > The function vnotify_conn_ex() notifies a player of an event.  A 
> > coordinate set can optionally be passed in to give the location of the 
> > event, or (-1,-1) can be passed to identify a "no-location" event.  This 
> > is a small hack, but nothing much to worry about.
> > There are two problems with the internals of vnotify_conn_ex(), though. 
> >  Both deal with this line:
> >     if (is_real_tile(x, y) && server_state >= RUN_GAME_STATE
> >
> > 1.  Calling is_real_tile before the game starts is generally no good. 
> > Under general-topologies, it will result in a failed assertion as the 
> > topology will not yet have been initialized.  Thus it should be clear 
> > that the order here should be reversed.
> > 2.  Again, is_real_tile may return true for (-1,-1) under an alternate 
> > topology.  The easy solution here is just to change it to 
> > is_normal_map_pos.  There are still some minor pitfalls: (1) it 
> > obviously won't work if is_normal_map_pos(-1,-1), and (2) we need to be 
> > careful about non-normal positions other than (-1,-1) being passed in; 
> > these would probably indicate a bug.  Both of these can be detected with 
> > a well-placed assertion.
> > The attached patch fixes both of these issues.
> >
> > Note, the handling here is very similar to that in Ross's corecleanups. 
> >  I believe this way to be slightly more correct, as it will catch the 
> > two pitfall potential bugs mentioned above.
> > jason
> > Index: server/plrhand.c
> > ===================================================================
> > RCS file: /home/freeciv/CVS/freeciv/server/plrhand.c,v
> > retrieving revision 1.203
> > diff -u -r1.203 plrhand.c
> > --- server/plrhand.c    2001/12/21 11:17:44     1.203
> > +++ server/plrhand.c    2002/01/02 08:41:17
> > @@ -893,7 +893,8 @@
> >   and specified (x,y) coords associated with the event.  Coords will only
> >   apply if game has started and the conn's player knows that tile (or
> >   pconn->player==NULL && pconn->observer).  If coords are not required,
> > -  caller should specify (x,y) = (-1,-1).  For generic event use E_NOEVENT.
> > +  caller should specify (x,y) = (-1,-1); otherwise make SURE the
> > +  coordinates have been normalized.  For generic event use E_NOEVENT.
> >   (But current clients do not use (x,y) data for E_NOEVENT events.)
> > **************************************************************************/
> > static void vnotify_conn_ex(struct conn_list *dest, int x, int y, int event,
> > @@ -905,12 +906,16 @@
> >   genmsg.event = event;
> >
> >   conn_list_iterate(*dest, pconn) {
> > -    if (is_real_tile(x, y) && server_state >= RUN_GAME_STATE
> > +    if (server_state >= RUN_GAME_STATE && is_normal_map_pos(x, y)
> >         && ((pconn->player==NULL && pconn->observer)
> >             || (pconn->player!=NULL && map_get_known(x, y, pconn->player)))) {
> > +      /* If is_normal_map_pos(-1,-1), this assertion will catch the bug. */
> > +      assert(x != -1 || y != -1);
> By definition this can't happen. But leaving it in doesn't hurt.
By current definition, yes.  I just want to be safe here.


> >       genmsg.x = x;
> >       genmsg.y = y;
> >     } else {
> > +      /* Any invalid coordinates other than (-1, -1) are a bug. */
> > +      assert((x == -1 && y == -1) || is_normal_map_pos(x, y));
> To satisfy Ross (he is correct on this) change it to:
>
> if(!(x == -1 && y == -1)) {
>   CHECK_MAP_POS(x,y);
> }
I don't think that'll be enough to satisfy Ross - it has identical 
effect to mine (at least with the current CHECK_MAP_POS).  His problems, 
as I understand them, are:
1.  Non-normal real positions are never allowed to be passed in.  This 
is a somewhat legitmate complaint.  On the one hand, we specify that all 
coordinates passed around must be normal, so why not do the same here? 
On the other, we have to call is_real_tile/is_normal_map_pos anyway, so 
we might as well call normalize_map_pos and make things a trifle safer 
(when compiled with NDEBUG).
2.  It may be possible for the client to trigger the CHECK_MAP_POS 
assertion here by sending a bad request to the server.  This is not a 
legitimate complaint IMO for two reasons.  (1) Doing this assertion 
check is unavoidable if we wish to catch the potential bug of non-normal 
coordinates being passed in, which we really must do.  (2) Passing in 
possibly bogus coordinates is a bug anyway, since this function won't 
handle them properly - non-real coordinates will be converted to 
(-1,-1), and non-normal real coordinates will either be normalized or 
converted to (-1,-1).
My most recent patch should satisfy him on point #1, but sticks by my 
stand on #2.

jason