[Freeciv-Dev] [bug #15305] memory leak in map_allocate

[pepeto]

Update of bug #15305 (project freeciv):

  Status:  Ready For Test => Fixed  
 Open/Closed:Open => Closed 


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


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

[Freeciv-Dev] [bug #15305] memory leak in map_allocate

[pepeto]

Update of bug #15305 (project freeciv):

  Status:None => Ready For Test 
 Assigned to:None => pepeto 
 Planned Release: => 2.2.1, 2.3.0   

___

Follow-up Comment #10:

Fix attached for both trunk and S2_2:
* initialize to NULL the pointers.
* use correct indentation and line break style.


(file #8150, file #8151)
___

Additional Item Attachment:

File name: trunk_map_allocate_mem_leak.diff Size:5 KB
File name: S2_2_map_allocate_mem_leak.diff Size:5 KB


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


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

[Freeciv-Dev] [bug #15305] memory leak in map_allocate

[pepeto]

Follow-up Comment #9, bug #15305 (project freeciv):

> Maybe nothing, I just tried to make a patch like you did and
> server crashed on map generation... But I didn't investigate
> more.

I may have done a mistake in my patch, I don't know where.  I tested your
patch with success.  However, it should be improved to initialize
city_map_index and map.iterate_outwards_indices, to avoid calls to random
datas (NULL ones would crashes, but at least, the bug would be clear).


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


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

[Freeciv-Dev] [bug #15305] memory leak in map_allocate

[pepeto]

Follow-up Comment #8, bug #15305 (project freeciv):

> Pepeto, what am I missing?

Maybe nothing, I just tried to make a patch like you did and server crashed
on map generation...  But I didn't investigate more.


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


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

[Freeciv-Dev] [bug #15305] memory leak in map_allocate

[anonymous]

Follow-up Comment #7, bug #15305 (project freeciv):

The server code is OK.

* The server calls server_game_free right before server_game_init in
server/stdinhand.c:load_command().  This frees any old game data which may
exist before loading a new game.  Note that all code paths which load saved
games (from the GGZ server, from argv options, and typing "load" at the
server prompt) go through load_command().

* The server calls server_game_free in server_quit, which is the way the
server exits.  Therefore game data is cleaned up before process exit.

* The server calls server_game_free at the bottom of the main game loop in
srv_main.  Therefore game data is cleared before you start a new game in the
same process.

* server_game_free calls game_free, which does map_free

Pepeto, what am I missing?


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


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

[Freeciv-Dev] [bug #15305] memory leak in map_allocate

[anonymous]

Follow-up Comment #6, bug #15305 (project freeciv):

Here is my second attempt at a patch.  This time around, the idea is to make
memory management for city.c : city_map_index and map.c :
map.iterate_outwards_indices work exactly the same way as it does for
map.tiles.

This time calls to realloc are replaced with calls to malloc, and some
asserts are added.  The asserts are simply to clarify the intent of the code,
and I have a strong argument as to why the assertions always hold.  Here
goes.

The memory for map.tiles, city_map_index, and map.iterate_outwards_indices is
allocated in map_allocate(), and with the patch it is released in map_free(). 
There is no scenario in which one of these should be allocated and any of the
others should not.  Hence the patch is correct assuming the calls to
map_allocate() and map_free() are themselves correct.  Are they?

Well, there are 3 scenarios in which map_allocate is called.  I have detailed
notes about this, but to make a long story short:

* The server calls map_allocate when a saved game is loaded
* The server calls map_allocate when a new map is generated
* The client calls map_allocate when it receives a PACKET_MAP_INFO

The map_free is function called in game_free, which is called in
server_game_free, and I don't think there is any indication right now there
is any problem with the server properly calling server_game_free after a game
is played.  Hence the server is OK.

The client is interesting because map_free is sometimes called before
map_allocate in the packet handler.  These calls are OK because map_free does
nothing if map_allocate hasn't been called yet.  The packet handling code also
performs a redundant check of the same condition in this case using
map_is_empty.

Anyway, the calls to map_free guarantee map_allocate won't leak if the server
sends multiple MAP_INFO packets during a game for some reason.  Additionally,
the client calls game_reset or game_free (which call map_free) when starting
new games or quitting, hence those cases are handled properly as well.

To sum up, map_allocate and map_free are called at appropriate times, so it
makes sense to do all the memory management for the map during those calls. 
That's what this patch does.

JKL

(file #8007)
___

Additional Item Attachment:

File name: map_allocate_memory_leak2.patch Size:4 KB


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


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

[Freeciv-Dev] [bug #15305] memory leak in map_allocate

[pepeto]

Follow-up Comment #5, bug #15305 (project freeciv):

Unfortunately, it seems it doesn't in server side...


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


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

[Freeciv-Dev] [bug #15305] memory leak in map_allocate

[pepeto]

Follow-up Comment #4, bug #15305 (project freeciv):

In current branches like S2_2 and trunk, I can affirm you that now the game
and the map are freed in client side when the client is disconnected; so
using malloc would be correct nowadays.  (I'm not sure about S2_1).


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


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

[Freeciv-Dev] [bug #15305] memory leak in map_allocate

[Matthias Pfafferodt]

Follow-up Comment #3, bug #15305 (project freeciv):

> I guess it wasn't a very good patch.

I would not say that. It is good to know that here a call to free() is
missing!

> The original code was written by jdorje and glip back in 2004.
> Hopefully they can tell us what is the right thing to do.

I found an older reference in the changelog:

> Tue Jun 15 11:46:47 1999  David Pfitzner :
>
> * client/packhand.c, common/map.c, common/map.h,
> server/mapgen.c, server/mapgen.h, server/maphand.c: Renamed
> init_workmap() in mapgen.c to map_allocate(), and moved to
> map.c.  Replaced duplicated code in
> packhand.c/handle_map_info() with call to map_allocate().  In
> map_allocate(), use realloc instead of malloc, for client. 
> (Previously there was a memory leak here in client when
> reconnecting multiple times to a running game).

So realloc() was used to prevent a memory leak in the client. If you want to
go on with your patch, you need to ensure that the allocation of these
variables are matched with a call to free().


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


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

[Freeciv-Dev] [bug #15305] memory leak in map_allocate

[anonymous]

Follow-up Comment #2, bug #15305 (project freeciv):

You are right about the reallocs.  Instead of calling this a "leak" I should
have said "map_allocate() relies on process exit to free some of its memory"
or something like that.  I admit I don't understand why realloc is used in
the index arrays instead of malloc/free.

Why are the index arrays treated differently than the tiles array?

By freeing the index arrays and setting them to NULL, the patch subverts the
original intent to reuse memory.  Therefore the reallocs probably also ought
to be changed to regular mallocs to make it clear what is happening, and the
comment about realloc in generate_citymap_index() should be deleted.

I guess it wasn't a very good patch.  The original code was written by jdorje
and glip back in 2004.  Hopefully they can tell us what is the right thing to
do.

JKL

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


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

[Freeciv-Dev] [bug #15305] memory leak in map_allocate

[Matthias Pfafferodt]

Follow-up Comment #1, bug #15305 (project freeciv):

good catch! It's best to free allocated memory within the code.

As I'm not so familiar with pointers and allocation of memory a question to
the functions used:

For both pointers (city_map_index and map.iterate_outwards_indices)
fc_realloc (freeciv wrapper for realloc) is used. If I'm right, this means
that, independent how often the functions are called, only one pointer with
the last requested memory size is allocated. So even if the allocations
happens quit often, it wouldn't be a big memory leak.

Comments to the patch:

* please do not insert trailing whitespaces to the code.
* freeciv also defines FC_FREE, a wrapper for free()

#define FC_FREE(ptr) do { free(ptr); (ptr) = NULL; } while(0)



___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


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

[Freeciv-Dev] [bug #15305] memory leak in map_allocate

[anonymous]

URL:
  

 Summary: memory leak in map_allocate
 Project: Freeciv
Submitted by: None
Submitted on: Monday 02/08/2010 at 00:07 CET
Category: general
Severity: 3 - Normal
Priority: 5 - Normal
  Status: None
 Assigned to: None
Originator Email: jkl102...@yahoo.com
 Open/Closed: Open
 Release: 
 Discussion Lock: Any
Operating System: None
 Planned Release: 

___

Details:

The map_allocate function calls the two functions

generate_city_map_indices()

and

generate_map_indices()

which allocate memory.  As far as I can tell, there is no corresponding code
which frees that memory.  Attached is a patch which modifies map_free to
perform the deallocation.  In the patch, it was necessary to modify the
city.c code as well because the city_map_index symbol is static.

JKL



___

File Attachments:


---
Date: Monday 02/08/2010 at 00:07 CET  Name: map_allocate_memory_leak.patch 
Size: 2kB   By: None



___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


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