[Freeciv-Dev] (PR#14638) Segfault at common/city.c:756

["Guillaume Melquiond" <guillaume.melquiond@xxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=14638 >

This transaction appears to have no content

The function city_can_be_built_here does the following test at line 756:
if (ptile->owner && ptile->owner != punit->owner)

Unfortunately, it doesn't check the validity of punit before deferencing it.
And punit can explicitely be null: "punit is the founding unit, which may be
NULL in some cases (e.g., cities from huts)". Grepping through the source
files shows that "cities from huts" is actually the only case where null can
happen. This is done in the function hut_get_city at
server/unitttools.c:2222.

As a consequence, if the tile of the hut is owned, the server crashes. The
caller is:
if (city_can_be_built_here(punit->tile, NULL))

But hut_get_city obviously knows the unit, so it should just pass it to
city_can_be_built_here. And this function would then simply assert that
punit is not null instead of trying to cope with it. Attached is a patch
against revision 11249.
The function city_can_be_built_here does the following test at line 756:
  if (ptile->owner && ptile->owner != punit->owner)

Unfortunately, it doesn't check the validity of punit before deferencing it. And punit can explicitely be null: "punit is the founding unit, which may be NULL in some cases (
e.g., cities from huts)". Grepping through the source files shows that "cities from huts" is actually the only case where null can happen. This is done in the function hut_get_city at server/unitttools.c:2222.

As a consequence, if the tile of the hut is owned, the server crashes. The caller is:
  if (city_can_be_built_here(punit->tile, NULL))

But hut_get_city obviously knows the unit, so it should just pass it to city_can_be_built_here. And this function would then simply assert that punit is not null instead of trying to cope with it. Attached is a patch against revision 11249.
Index: server/unittools.c
===================================================================
--- server/unittools.c  (révision 11249)
+++ server/unittools.c  (copie de travail)
@@ -2219,7 +2219,7 @@
 {
   struct player *pplayer = unit_owner(punit);
 
-  if (city_can_be_built_here(punit->tile, NULL)) {
+  if (city_can_be_built_here(punit->tile, punit)) {
     notify_player(pplayer, punit->tile, E_HUT_CITY,
                     _("You found a friendly city."));
     create_city(pplayer, punit->tile,
Index: common/city.c
===================================================================
--- common/city.c       (révision 11249)
+++ common/city.c       (copie de travail)
@@ -735,19 +735,20 @@
 
 /**************************************************************************
   Returns TRUE if the given unit can build a city at the given map
-  coordinates.  punit is the founding unit, which may be NULL in some
-  cases (e.g., cities from huts).
+  coordinates.  punit is the founding unit.
 **************************************************************************/
 bool city_can_be_built_here(const struct tile *ptile, const struct unit *punit)
 {
   int citymindist;
 
+  assert(punit);
+
   if (terrain_has_flag(ptile->terrain, TER_NO_CITIES)) {
     /* No cities on this terrain. */
     return FALSE;
   }
 
-  if (punit && !can_unit_exist_at_tile(punit, ptile)) {
+  if (!can_unit_exist_at_tile(punit, ptile)) {
     /* We allow land units to build land cities and sea units to build
      * ocean cities. Air units can build cities anywhere. */
     return FALSE;