Re: [Freeciv-Dev] (PR#40176) map_distance_vector() crash

2008-04-03 Thread Marko Lindqvist 

http://bugs.freeciv.org/Ticket/Display.html?id=40176 >

On 03/04/2008, Marko Lindqvist  wrote:
>
> On 03/04/2008, Marko Lindqvist  wrote:
>  >   This should be fixable simply by adding ref_count for vision sites.
>
>  Patch attached.

 - Handle reassigning same vision site to tile correctly
 - Some more comments added



 - ML

diff -Nurd -X.diff_ignore freeciv/common/vision.c freeciv/common/vision.c
--- freeciv/common/vision.c	2008-03-08 16:32:49.0 +0200
+++ freeciv/common/vision.c	2008-04-03 03:01:51.0 +0300
@@ -17,6 +17,7 @@
 
 #include 
 
+#include "log.h"
 #include "mem.h"
 #include "shared.h"
 
@@ -77,6 +78,11 @@
 /
 void free_vision_site(struct vision_site *psite)
 {
+  if (psite->ref_count > 0) {
+/* Somebody still uses this vision site. Do not free */
+return;
+  }
+  assert(psite->ref_count == 0);
   free(psite);
 }
 
@@ -119,3 +125,25 @@
   psite->size = pcity->size;
   sz_strlcpy(psite->name, city_name(pcity));
 }
+
+/
+  Copy relevant information from one site struct to another.
+  Currently only user expects everything except ref_count to be copied.
+  If other kind of users are added, parameter defining copied information
+  set should be added.
+/
+void copy_vision_site(struct vision_site *dest, struct vision_site *src)
+{
+  /* Copy everything except ref_count. */
+  strcpy(dest->name, src->name);
+  dest->location = src->location;
+  dest->owner = src->owner;
+  dest->identity = src->identity;
+  dest->size = src->size;
+  dest->border_radius_sq = src->border_radius_sq;
+  dest->occupied = src->occupied;
+  dest->walls = src->walls;
+  dest->happy = src->happy;
+  dest->unhappy = src->unhappy;
+  dest->improvements = src->improvements;
+}
diff -Nurd -X.diff_ignore freeciv/common/vision.h freeciv/common/vision.h
--- freeciv/common/vision.h	2008-03-08 16:32:49.0 +0200
+++ freeciv/common/vision.h	2008-04-03 00:05:41.0 +0300
@@ -124,6 +124,8 @@
   bool happy;
   bool unhappy;
 
+  int ref_count;
+
   bv_imprs improvements;
 };
 
@@ -134,6 +136,7 @@
 struct vision_site *create_vision_site_from_city(const struct city *pcity);
 void update_vision_site_from_city(struct vision_site *psite,
   const struct city *pcity);
+void copy_vision_site(struct vision_site *dest, struct vision_site *src);
 
 /* get 'struct site_list' and related functions: */
 #define SPECLIST_TAG site
diff -Nurd -X.diff_ignore freeciv/server/citytools.c freeciv/server/citytools.c
--- freeciv/server/citytools.c	2008-03-12 21:58:27.0 +0200
+++ freeciv/server/citytools.c	2008-04-03 03:48:32.0 +0300
@@ -1562,6 +1562,8 @@
   continue;
 }
 whole_map_iterate(ptile) {
+  /* FIXME: map_get_player_base()???
+   * Should be cities only: map_get_player_city() */
   if (!pplayer || NULL != map_get_player_base(ptile, pplayer)) {
 	send_city_info_at_tile(pplayer, pconn->self, NULL, ptile);
   }
@@ -1791,8 +1793,8 @@
   } improvement_iterate_end;
 
   if (NULL == pdcity) {
-map_get_player_tile(pcenter, pplayer)->site =
 pdcity = create_vision_site_from_city(pcity);
+change_playertile_site(map_get_player_tile(pcenter, pplayer), pdcity);
   } else if (pdcity->location != pcenter) {
 freelog(LOG_ERROR, "Trying to update bad city (wrong location)"
 	" at %i,%i for player %s",
@@ -1840,8 +1842,12 @@
   struct player_tile *playtile = map_get_player_tile(ptile, pplayer);
 
   dlsend_packet_city_remove(pplayer->connections, pdcity->identity);
+  assert(playtile->site == pdcity);
   playtile->site = NULL;
-  free(pdcity);
+  pdcity->ref_count--; /* Subtract ref_count before calling
+* free_vision_site() */
+  assert(pdcity->ref_count >= 0);
+  free_vision_site(pdcity);
 }
   }
 }
diff -Nurd -X.diff_ignore freeciv/server/maphand.c freeciv/server/maphand.c
--- freeciv/server/maphand.c	2008-03-12 21:58:27.0 +0200
+++ freeciv/server/maphand.c	2008-04-03 11:06:06.0 +0300
@@ -1073,6 +1073,34 @@
 }
 
 /***
+ Changes site information for player tile.
+***/
+void change_playertile_site(struct player_tile *ptile,
+struct vision_site *new_site)
+{
+  if (ptile->site == new_site) {
+/* Do nothing. Especially: don't decrease ref_count and
+ * free vision site... */
+return;
+  }
+
+  if (ptile->site != NULL) {
+/* Releasing old site from tile */
+ptile->site->ref_count--;
+assert(ptile->site->ref_count >= 0);
+if (ptile->site->ref_count == 0) {
+  /* Free vision site before losing its address completely */
+  free_vision_site(ptile->site);
+}
+  }
+  if (new_site != NUL

Re: [Freeciv-Dev] (PR#40176) map_distance_vector() crash

2008-04-02 Thread Marko Lindqvist 

http://bugs.freeciv.org/Ticket/Display.html?id=40176 >

On 03/04/2008, Marko Lindqvist  wrote:
>   This should be fixable simply by adding ref_count for vision sites.

 Patch attached.


 - ML

diff -Nurd -X.diff_ignore freeciv/common/vision.c freeciv/common/vision.c
--- freeciv/common/vision.c	2008-03-08 16:32:49.0 +0200
+++ freeciv/common/vision.c	2008-04-03 03:01:51.0 +0300
@@ -17,6 +17,7 @@
 
 #include 
 
+#include "log.h"
 #include "mem.h"
 #include "shared.h"
 
@@ -77,6 +78,11 @@
 /
 void free_vision_site(struct vision_site *psite)
 {
+  if (psite->ref_count > 0) {
+/* Somebody still uses this vision site. Do not free */
+return;
+  }
+  assert(psite->ref_count == 0);
   free(psite);
 }
 
@@ -119,3 +125,25 @@
   psite->size = pcity->size;
   sz_strlcpy(psite->name, city_name(pcity));
 }
+
+/
+  Copy relevant information from one site struct to another.
+  Currently only user expects everything except ref_count to be copied.
+  If other kind of users are added, parameter defining copied information
+  set should be added.
+/
+void copy_vision_site(struct vision_site *dest, struct vision_site *src)
+{
+  /* Copy everything except ref_count. */
+  strcpy(dest->name, src->name);
+  dest->location = src->location;
+  dest->owner = src->owner;
+  dest->identity = src->identity;
+  dest->size = src->size;
+  dest->border_radius_sq = src->border_radius_sq;
+  dest->occupied = src->occupied;
+  dest->walls = src->walls;
+  dest->happy = src->happy;
+  dest->unhappy = src->unhappy;
+  dest->improvements = src->improvements;
+}
diff -Nurd -X.diff_ignore freeciv/common/vision.h freeciv/common/vision.h
--- freeciv/common/vision.h	2008-03-08 16:32:49.0 +0200
+++ freeciv/common/vision.h	2008-04-03 00:05:41.0 +0300
@@ -124,6 +124,8 @@
   bool happy;
   bool unhappy;
 
+  int ref_count;
+
   bv_imprs improvements;
 };
 
@@ -134,6 +136,7 @@
 struct vision_site *create_vision_site_from_city(const struct city *pcity);
 void update_vision_site_from_city(struct vision_site *psite,
   const struct city *pcity);
+void copy_vision_site(struct vision_site *dest, struct vision_site *src);
 
 /* get 'struct site_list' and related functions: */
 #define SPECLIST_TAG site
diff -Nurd -X.diff_ignore freeciv/server/citytools.c freeciv/server/citytools.c
--- freeciv/server/citytools.c	2008-03-12 21:58:27.0 +0200
+++ freeciv/server/citytools.c	2008-04-03 03:32:24.0 +0300
@@ -1562,6 +1562,8 @@
   continue;
 }
 whole_map_iterate(ptile) {
+  /* FIXME: map_get_player_base()???
+   * Should be cities only: map_get_player_city() */
   if (!pplayer || NULL != map_get_player_base(ptile, pplayer)) {
 	send_city_info_at_tile(pplayer, pconn->self, NULL, ptile);
   }
@@ -1791,8 +1793,8 @@
   } improvement_iterate_end;
 
   if (NULL == pdcity) {
-map_get_player_tile(pcenter, pplayer)->site =
 pdcity = create_vision_site_from_city(pcity);
+change_playertile_site(map_get_player_tile(pcenter, pplayer), pdcity);
   } else if (pdcity->location != pcenter) {
 freelog(LOG_ERROR, "Trying to update bad city (wrong location)"
 	" at %i,%i for player %s",
@@ -1840,8 +1842,11 @@
   struct player_tile *playtile = map_get_player_tile(ptile, pplayer);
 
   dlsend_packet_city_remove(pplayer->connections, pdcity->identity);
+  assert(playtile->site == pdcity);
   playtile->site = NULL;
-  free(pdcity);
+  pdcity->ref_count--; /* Subtract ref_count before calling
+* free_vision_site() */
+  free_vision_site(pdcity);
 }
   }
 }
diff -Nurd -X.diff_ignore freeciv/server/maphand.c freeciv/server/maphand.c
--- freeciv/server/maphand.c	2008-03-12 21:58:27.0 +0200
+++ freeciv/server/maphand.c	2008-04-03 03:36:35.0 +0300
@@ -1073,6 +1073,28 @@
 }
 
 /***
+ Changes site information for player tile.
+***/
+void change_playertile_site(struct player_tile *ptile,
+struct vision_site *new_site)
+{
+  if (ptile->site != NULL) {
+/* Releasing old site from tile */
+ptile->site->ref_count--;
+assert(ptile->site->ref_count >= 0);
+if (ptile->site->ref_count == 0) {
+  /* Free vision site before losing its address completely */
+  free_vision_site(ptile->site);
+}
+  }
+  if (new_site != NULL) {
+/* Assigning new site to tile */
+new_site->ref_count++;
+  }
+  ptile->site = new_site;
+}
+
+/***
 ...
 ***/
 void map_set_known(struct tile *ptile, struct player *pplayer)
@@ -1148,8 +

Re: [Freeciv-Dev] (PR#40176) map_distance_vector() crash

2008-04-02 Thread Marko Lindqvist 

http://bugs.freeciv.org/Ticket/Display.html?id=40176 >

On 01/04/2008, Marko Lindqvist  wrote:
>  #1  0x0040be1c in sq_map_distance (tile0=0x14, 
> tile1=0xbf9638)

 As interesting value of tile0 indicates...

>  #2  0x0048f1f4 in map_claim_border (ptile=0xbfb1b8, powner=0x896100)
> at ../../../src.patched/server/maphand.c:1931
> dsite = (struct vision_site *) 0xb37ef0

 ...dsite points to random data.


 I'm quite sure this is because vision site has been freed.

 Several player tiles point to same vision site. AFAICT possible other
pointers to vision site are not considered when one tile decides to
free() it. I have to admit that I really don't understand the logic of
tiles free()ing vision site in some situations and not free()ing in
other situations before they lose its address.

 Anyway, after I tried to simply comment out vision site free(),
crashing stopped (obviously there is big memory leak instead).
 This should be fixable simply by adding ref_count for vision sites.


 - ML



___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev