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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

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.

>        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);
}

>        genmsg.x = -1;
>        genmsg.y = -1;
>      }


        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  This customer comes into the computer store. "I'm looking for a mystery
  Adventure Game with lots of graphics. You know, something realy
  challenging". "Well," replied the clerk, "have you tried Windows 98 ?"