[Freeciv-Dev] Re: bug with initialization of goto_dest_[xy] (PR#1190)

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

This has the same problem as the previous buglet. Misuse of 
is_normal_map_pos() as an incomplete partial test of a soft condition, 
plus the introduction of buggy or ambiguous followon behaviour as a 
result. 

The correct test is is_real_tile() as realness is a hard condition that
affects the game, (soft conditions can always be removed).

If the coordinates are real, then they may need to be normalized to be
used (i.e. in memory lookups).

normalize_map_pos() satisfies both the test and the fix, and is the
appropriate function to use here.

Note, a correctly functionning normalize_map_pos() will return FALSE
for an unreal position, and will correctly map the coordinates if they
are real. A correctly functionning normalize_map_pos() should not modify 
the parameters it has been passed unless it is safe to do so. This means 
it doesn't introduce any complicating side effects in the unreal one.

Just checking for normalization misses the case were the coordinates are
real, but not yet normalized, and so incorrectly deals with the 
followon logic (i.e. treats them as unreal which is clearly a bug). 

Cheers,
RossW
=====

At 02:16 AM 02/01/02 -0500, Jason Short wrote:
>The values punit->goto_dest_x and punit->goto_dest_y are never 
>"initialized" when a unit is created, and there's no way to indicate 
>that the unit has no goto destination.  Rather, on initialization the 
>values just get set to (0,0) and later may (perhaps?) be reset to this 
>later if the goto destination is removed.
>
>Code that uses the goto values generally compares them to something, or 
>looks to see what's there (a city, an enemy unit, etc.), and ignores the 
>goto if not.  Since the tile (0,0) is generally unusable, this is fairly 
>safe, although it is far from "correct".
>
>Under a non-rectangular topology, this hack leads to a bug.  For 
>instance, under an iso-rectanguler topology (0,0) is generally not a 
>"normal" map position, so CHECK_MAP_POS will fail and there will be a 
>segfault.  So, one solution is to hack the hack so that we call 
>is_normal_map_pos to check for this case before using the coordinates 
>(this is only necessary in a few cases).  As long as (0,0) always 
>remains an unusable OR non-normal position, the hack should continue to 
>work.  The attached patch makes this change; it is the simplest one 
>possible AFAICT.
>
>The "correct" solution would IMO be to add another piece of data to 
>track whether the goto_dest is valid at all.  I can think of two ways to 
>do this: (1) make the goto_dest a struct map_position pointer, which is 
>set at NULL when the goto_dest is not in use and malloc'ed as necessary, 
>or (2) create a separate field goto_dest_is_used (or the like) that can 
>store the validity information for the position.
>
>I really think we should go for the correct solution here.  However, it 
>will be a non-trivial task, so I've only provided this small fix for now.
>
>Note, all of this is closely tied to goto_is_sane() in gotohand.c. 
>However, goto_is_sane will segfault if given non-normal coordinates, so 
>even if we use that function to check coordinates we'll have to fix it up.
>
>Note also, Ross's corecleanups attempt to fix these pieces of code by 
>calling normalize_map_pos() on the goto_dest map position.  This is a 
>bug (and a nasty, one, too!), since it may wrap the invalid position to 
>a valid one, thus causing the goto to be set to some random position 
>somewhere on the map!
>
>jason
>Index: ai/aiunit.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.c,v
>retrieving revision 1.158
>diff -u -r1.158 aiunit.c
>--- ai/aiunit.c        2001/11/05 01:59:59     1.158
>+++ ai/aiunit.c        2002/01/02 07:03:30
>@@ -1628,7 +1628,24 @@
>       if (!punit->ai.passenger) punit->ai.passenger = aunit->id; /* oops */
>       p++;
>       bodyguard = unit_list_find(&map_get_tile(punit->x,
punit->y)->units, aunit->ai.bodyguard);
>-      pcity = map_get_city(aunit->goto_dest_x, aunit->goto_dest_y);
>+
>+      /*
>+       * HACK: we currently don't initialize (goto_dest_x, goto_dest_y) at
>+       * all; they just get set to (0,0) and we hope that this value never
>+       * actualy gets used (which it shouldn't, given where it lies).  But
>+       * under a general topology system (0,0) may not even be a normal
>+       * map position, which will result in a segfault if it's used.  This
>+       * hack just avoids the crash, it doesn't cure the more general
>+       * problem of having invalid goto coordinates.
>+       *
>+       * This same hack is also used in is_already_assigned() in settlers.c.
>+       */
>+      if (is_normal_map_pos(aunit->goto_dest_x, aunit->goto_dest_y)) {
>+        pcity = map_get_city(aunit->goto_dest_x, aunit->goto_dest_y);
>+      } else {
>+        pcity = NULL;
>+      }
>+
>       if (!aunit->ai.bodyguard || bodyguard ||
>          (pcity && pcity->ai.invasion >= 2)) {
>       if (pcity) {
>Index: server/settlers.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/server/settlers.c,v
>retrieving revision 1.115
>diff -u -r1.115 settlers.c
>--- server/settlers.c  2001/12/06 11:59:07     1.115
>+++ server/settlers.c  2002/01/02 07:03:33
>@@ -405,8 +405,11 @@
> **************************************************************************/
> static int is_already_assigned(struct unit *myunit, struct player
*pplayer, int x, int y)
> {
>+  /* See the HACK comment in ai_manage_ferryboat to see why we
>+     use is_normal_map_pos() here. */
>   if (same_pos(myunit->x, myunit->y, x, y) ||
>-      same_pos(myunit->goto_dest_x, myunit->goto_dest_y, x, y)) {
>+      (is_normal_map_pos(myunit->goto_dest_x, myunit->goto_dest_y)
>+       && same_pos(myunit->goto_dest_x, myunit->goto_dest_y, x, y))) {
> /* I'm still not sure this is exactly right -- Syela */
>     unit_list_iterate(map_get_tile(x, y)->units, punit)
>       if (myunit==punit) continue;
>