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

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 03:44 AM 02/01/02 -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.

Yes. But not to cover up bugs in the process of being designed into 
upcoming topology fixes. It is simply more efficient, plus avoids the 
problem that maps may not yet be initialized in the current code :-).

>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. 

Since (-1, -1) is allowed either as a real value, or a hack flag, it is
sufficient to simply catch it and fall through the alternate clause.

If is_real_tile() is no longer sufficient in the general case, then 
normalize_map_pos() should be used to guarantee that the coordinate is 
correct for the subsequent map_get_known() call under the new rules. In 
the past this would all have been handled by map_get_known(), so treating 
this as an error would introduce different behaviour and a potential bug.

The fact that there was a check for a hard condition "realness" and the
soft condition "normalized" was handled in map_get_known() is the key
indicator that a check for the meaningless soft condition (it has no
useful game value in itself, i.e. without knowing realness) is probably 
not sufficient.

Remember, is_normal_map_pos() was only deemed useful in asserts and 
allowed for the case of CHECK_POS(), i.e. to find passed coordinates 
now considered bogus during the process of debugging the new policy. 

The general rule is ... 
is_normal_map_pos() use is almost always a mistake or hiding one :-).

> 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 use of assertions is really unnecessary. This is a reasonable point
to do full checks and fixups as an entry/exit data portal. Killing a server
everytime a client does something bogus is not good form.

The original code did full checks. There might actually be a reason for 
this, other than happenstance. Certainly it has not caused problems, so
there is no reason to change it here.

>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.

The fact that it uses is_normal_pos() is a leading indicator of 
incorrectness :-).

This should fix all the issues in a backwards compatible way without
introducing any new problems or ways to kill the server.

    /* E_NOEVENT called map_get_known() with (-1,-1) in Torus world */
    if (server_state >= RUN_GAME_STATE
     && ((x != -1) || (y != -1))
     && normalize_map_pos(&x, &y)             /* was is_real_tile() */
     && ((pconn->player==NULL && pconn->observer)
        || (pconn->player!=NULL && map_get_known(x, y, pconn->player)))) {
      genmsg.x = x;
      genmsg.y = y;
    } else {
      genmsg.x = -1;
      genmsg.y = -1;
    }
    send_packet_generic_message(pconn, PACKET_CHAT_MSG, &genmsg);
   

>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);
>       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));
>       genmsg.x = -1;
>       genmsg.y = -1;
>     }
>