[Cabal] [Fwd: xmlnode memory leak patch]

[Nathan Walp]

Ethan Blanton wrote:
> A fellow who's using some Gaim code for GNUnet sent me this patch ...
> Nathan, I believe you wrote the xmlnode stuff, can you take a peek at
> it?  It looks pretty reasonable to me, but he says there are comments
> that say the pool allocation stuff was *removed* from xmlnode at some
> point, and I suspect there was a reason.
> 
> Ethan

It was removed because we didn't really need to allocators (glib and 
non-glib) in gaim.  The Jabber plugin used to have a ton of stuff that 
would do

char *bar = g_strdup(foo);
p_free(foo);
return bar;

...and that just got to be too much.


Anyways, this guy has not implemented a memory pool, so for starters I 
don't like his patch because he gets the terminology wrong.  Let me 
summarize what the old code does, and what the new code is doing as best 
I can tell:

Old code:

Allocate and free nodes as they're added and removed.  Every node keeps 
track of it's parent and children.  If you free a parent, you free all 
of its children, then itself.

In the error condition he talks about, we go back out to the initial 
node (the parent's parent's parent's parent's parent's parent's 
parent's........parent), and then free it, which will free all of its 
children, then free itself.

New code (my interpretation):

Allocate nodes as they're added.  When the initial node is added, also 
allocate another structure to keep a pointer to every node created.  If 
a node is removed, don't deallocate it.  If the initial node is free'd, 
then use that list of pointer to free all the nodes.

In the error condition, use that list of pointers to free all of the nodes.


I really can't figure out what errant behavior his patch is supposed to 
"fix" since the leak he describes is already handled.  Actually, his 
patch fails to GAIM_DBUS_UNREGISTER the pointers.

Actually, I just looked closer, and he uses pool to refer to two 
completely different data structures, gets himself confused, and ends up 
corrupting the crap out of memory.

I won't be applying this patch.  If he can provide a real leak case, 
I'll be glad to take a look at it, but this is a poorly implemented 
overkill patch for a leak that isn't there.

Wow, I should go eat something.  I'm cranky.

-Nathan