[Freeciv-Dev] bug with initialization of goto_dest_[xy] (PR#1184)

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

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;