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

[Jason Short <vze2zq63@xxxxxxxxxxx>]

jdorje@xxxxxxxxxxxxxxxxxxxxx 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. 
This patch is a compromise between Ross's solution and mine.

- I use normalize_map_pos rather than is_normal_map_pos, so if a 
non-normal map position is passed in we'll be able to deal with it fine 
(so long as it's not (-1,-1)).  This is basically identical to Ross's 
changes.
- I've added two extra assertions at the top to catch the two potential 
bugs I mentioned previously.  These assertions are crucial; without them 
any buggy code that passes in non-normal positions to this function will 
result in a nearly untraceable bug on the client end.  NOTE, the use of 
assert(is_normal(...)) rather than CHECK_MAP_POS(...) is quite 
intentional - CHECK_MAP_POS may attempt to do error-correction or other 
fixes rather than a simple assert, which would destroy the point of the 
code.
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/05 07:39:57
@@ -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,
@@ -904,8 +905,27 @@
   my_vsnprintf(genmsg.message, sizeof(genmsg.message), format, vargs);
   genmsg.event = event;
 
+  /*
+   * Hack: we special-case the coordinates (-1,-1) to mean the event has
+   * no coordinates associated with it.  This works just fine, so long
+   * as (-1,-1) isn't a "real" map position.  But if we use a topology
+   * where it is a real position, then we have a major problem.  If the
+   * calling code ever passes in (-1,-1) as the legitimate coordinates of
+   * the event, we'll end up treating them as the special-case so we'll
+   * think there are no coordinates associated with the event.  There's
+   * really no way around this bug, but we can try to catch the error by
+   * making sure non-normal positions never get passed in.
+   */
+  if (x == -1 && y == -1) {
+    assert(!is_normal_map_pos(x, y));
+  } else {
+    assert(is_normal_map_pos(x, y));
+  }
+
   conn_list_iterate(*dest, pconn) {
-    if (is_real_tile(x, y) && server_state >= RUN_GAME_STATE
+    if (server_state >= RUN_GAME_STATE
+        && (x != -1 || y != -1) /* special case, see above */
+        && normalize_map_pos(&x, &y)
        && ((pconn->player==NULL && pconn->observer)
            || (pconn->player!=NULL && map_get_known(x, y, pconn->player)))) {
       genmsg.x = x;