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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sat, Jan 05, 2002 at 06:41:25PM -0500, Jason Short wrote:
> 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).

Is someone compiles with NDEBUG he says that he goes for raw speed. He
can't expect a graceful landing if an error occurs. So I don't see a
problem here.

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

Ack. This is an _output_ function. Bogus user-input values should be
catched in an _input_ function.

> My most recent patch should satisfy him on point #1, but sticks by my 
> stand on #2.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "How about the new language C&? No, that's not 'c ampersand', 'c reference', 
  'reference to c' or 'c and'. It's pronounced 'campersand', to confuse the 
  hell out of people who are unfamiliar with it, and it will, of course, 
  have no pointers."
    -- Xazziri in comp.lang.c++ about C#