Complete.Org: Mailing Lists: Archives: freeciv-dev: January 2002:
[Freeciv-Dev] Re: [PATCH] bug in vnotify_conn_ex()
Home

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

[Top] [All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
Cc: freeciv-dev@xxxxxxxxxxx
Subject: [Freeciv-Dev] Re: [PATCH] bug in vnotify_conn_ex()
From: Jason Short <vze2zq63@xxxxxxxxxxx>
Date: Sat, 05 Jan 2002 18:41:25 -0500
Reply-to: jdorje@xxxxxxxxxxxx

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



[Prev in Thread] Current Thread [Next in Thread]