Re: GLIB - gnode.c - g_node_destroy(): Feature or error?

[otaylor fresnel labs redhat com]

tobias badertscher phim unibe ch writes:

> First: Sorry for my long mail.
> 
> In a program I wrote I start at the leaf and delete nodes (maybe) down
> to the root (depends on the node data). So it happend that at the end of
> my subroutine when I destroy the tree, I deleted the root node twice
> which could result (glib-1.2.7 or glib-1.2.8, gcc 2.95.2, SuSE 6.4) that
> my Window Manager gets killed.

My guess is that either that your stack size limit is very big or
you are tight on memory - in either case, the problem is that your
bug puts GTK+ into an infinite recursion.

>  The error can be reproduced by the following program:


> <----------------- SNIPE ------------------------->
> #include <glib.h>
> int main(int argc, char** argv){
> 	GNode	* root=NULL;
> 	GNode	* tree=NULL;
> 	gint	* data;
> 	
> 	data		=	g_new(gint,1);
> 	*data		=	1;
> 	
> 	root	=	g_node_new(data);
> 	g_print("root       : %d\n",(gint)* (gpointer *)(root->data));
> 	
> 	data		=	g_new(gint,1);
> 	*data		=	2;
> 	tree	=	g_node_append_data(root,data);	
> 	g_print("1st sibling: %d\n",(gint)* (gpointer *)tree->data);
> 	
> 	data		=	g_new(gint,1);
> 	*data		=	3;
> 	tree	=	g_node_append_data(root,data);	
> 	g_print("2nd sibling: %d\n",(gint)* (gpointer *)tree->data);
> 
> 	data		=	g_new(gint,1);
> 	*data		=	3;
> 	tree	=	g_node_append_data(tree,data);	
> 	g_print("1st node of 2nd sibling: %d\n",(gint)* (gpointer
> *)tree->data);
> 	
> 	g_node_destroy(root);
> 	g_node_destroy(root);

[...]

> <----------------------SNIPE--------------------->
> So - It is probably bad programming style to delete the root node twice,
> but if glib could crash under such circumstances a user session....

It certainly is not "bad programming style", it is a plain old bug.
You cannot free a node twice, period.
 
> If glib is changed according to my suggestion there is still an other
> problem. If the root node is destroyed twice there are two copies of the
> root node memory adresse in the  gnodes 'current_allocator' data
> structure. If later two new GNodes are fetched (g_node_new), you get
> twice the same memory adresse. 

Freeing the same piece of memory twice is a bug, no matter whether 
it is a list node, a tree node, a malloc()'ed block...

What your patch does is substitutes confusing slow corruption for an
immediate crash. 

I've had to debug multiply freed list nodes a few times and it isn't
at all fun, since you eventually end up with a list node that is used
twice, and you get weird bugs that are completely unrelated to the
source of the problem.

(However, we are going to add a --enable-gc-friendly configuration
option to GLib which clears all sorts of freed memory, so that will
be a bit like your patch.)

Regards,
                                        Owen