[Cabal] [Fwd: xmlnode memory leak patch]
faceprint at faceprint.com
Sun Dec 31 12:10:40 EST 2006
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.
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
char *bar = g_strdup(foo);
...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:
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.
More information about the Cabal