From Andrew Narver:

Currently, if you call proto_tree_free on anything other than the root node of a tree
the tree will get left in an inconsistent state.  This is because the parent is left pointing
to the newly freed child.

The traversal code is updated, the parent node update is currently disabled since
freeing is done for the complete tree only at this time, so there is no need to keep
the parent node consistent.

git-svn-id: http://anonsvn.wireshark.org/wireshark/trunk@26466 f5534014-38df-0310-8fa8-9805f1628bb7

diff --git a/epan/dfilter/dfilter-macro.c b/epan/dfilter/dfilter-macro.c
index 832190131e1b5a475ba352f79550b6eb3e6e9442..319754c187b72edf4bb7b5188156be2dd89bfd3c 100644 (file)
--- a/epan/dfilter/dfilter-macro.c
+++ b/epan/dfilter/dfilter-macro.c
@@ -93,7 +93,7 @@ static gboolean fvt_cache_cb(proto_node * node, gpointer data _U_) {
 
 void dfilter_macro_build_ftv_cache(void* tree_root) {
        g_hash_table_foreach_remove(fvt_cache,free_value,NULL);
-       proto_tree_traverse_in_order(tree_root, fvt_cache_cb, NULL);
+       proto_tree_traverse_post_order(tree_root, fvt_cache_cb, NULL);
 }
 
 void dfilter_macro_foreach(dfilter_macro_cb_t cb, void* data) {

diff --git a/epan/proto.c b/epan/proto.c
index 9d24a42b9fe3cfb7048fe182bd0d98f8a377e02a..4a26baed5f04dc39334e7f3a54f75b30164925d6 100644 (file)
--- a/epan/proto.c
+++ b/epan/proto.c
@@ -440,7 +440,7 @@ proto_tree_traverse_pre_order(proto_tree *tree, proto_tree_traverse_func func,
 }
 
 gboolean
-proto_tree_traverse_in_order(proto_tree *tree, proto_tree_traverse_func func,
+proto_tree_traverse_post_order(proto_tree *tree, proto_tree_traverse_func func,
     gpointer data)
 {
        proto_node *pnode = tree;
@@ -448,7 +448,7 @@ proto_tree_traverse_in_order(proto_tree *tree, proto_tree_traverse_func func,
        proto_node *current;
 
        child = pnode->first_child;
-       if (child != NULL) {
+       while (child != NULL) {
                /*
                 * The routine we call might modify the child, e.g. by
                 * freeing it, so we get the child's successor before
@@ -456,30 +456,12 @@ proto_tree_traverse_in_order(proto_tree *tree, proto_tree_traverse_func func,
                 */
                current = child;
                child = current->next;
-
-               if (proto_tree_traverse_in_order((proto_tree *)current, func,
+               if (proto_tree_traverse_post_order((proto_tree *)current, func,
                    data))
                        return TRUE;
-
-               if (func(pnode, data))
-                       return TRUE;
-
-               while (child != NULL) {
-                       /*
-                        * The routine we call might modify the child, e.g. by
-                        * freeing it, so we get the child's successor before
-                        * calling that routine.
-                        */
-                       current = child;
-                       child = current->next;
-                       if (proto_tree_traverse_in_order((proto_tree *)current,
-                           func, data))
-                               return TRUE;
-               }
-       } else {
-               if (func(pnode, data))
-                       return TRUE;
        }
+       if (func(pnode, data))
+               return TRUE;
 
        return FALSE;
 }
@@ -503,7 +485,7 @@ proto_tree_children_foreach(proto_tree *tree, proto_tree_foreach_func func,
 void
 proto_tree_free(proto_tree *tree)
 {
-       proto_tree_traverse_in_order(tree, proto_tree_free_node, NULL);
+       proto_tree_traverse_post_order(tree, proto_tree_free_node, NULL);
 }
 
 static void
@@ -556,11 +538,14 @@ static gboolean
 proto_tree_free_node(proto_node *node, gpointer data _U_)
 {
        field_info *finfo = PITEM_FINFO(node);
+#if 0
+       proto_node *parent = node->parent;
+#endif
 
        if (finfo == NULL) {
                /* This is the root node. Destroy the per-tree data.
                 * There is no field_info to destroy. */
-               free_node_tree_data(PTREE_DATA(node));
+               if (PTREE_DATA(node)) free_node_tree_data(PTREE_DATA(node));
        }
        else {
                /* This is a child node. Don't free the per-tree data, but
@@ -568,6 +553,33 @@ proto_tree_free_node(proto_node *node, gpointer data _U_)
                FREE_NODE_FIELD_INFO(finfo);
        }
 
+#if 0
+       /* NOTE: This code is required when this function is used to free individual
+        * nodes only. Current use is for the destruction of complete trees, so the
+        * inconsistancies have no ill effect.
+        */
+       /* Remove node from parent */
+       if (parent) {
+               proto_item *prev_item = NULL;
+               if (parent->first_child == node) {
+                       parent->first_child = node->next;
+               } else {
+                       /* find previous and change its next */
+                       for (prev_item = parent->first_child; prev_item; prev_item = prev_item->next) {
+                               if (prev_item->next == node) {
+                                       break;
+                               }
+                       }
+                       DISSECTOR_ASSERT(prev_item);
+                       prev_item->next = node->next;
+               }
+               /* fix last_child if required */
+               if (parent->last_child == node) {
+                       parent->last_child = prev_item;
+               }
+       }
+       DISSECTOR_ASSERT(node->first_child == NULL && node->last_child == NULL);
+#endif
        /* Free the proto_node. */
        PROTO_NODE_FREE(node);
 
@@ -3373,8 +3385,8 @@ proto_tree_get_root(proto_tree *tree) {
 void
 proto_tree_move_item(proto_tree *tree, proto_item *fixed_item, proto_item *item_to_move)
 {
-    proto_item *curr_item;
-
+    DISSECTOR_ASSERT(item_to_move->parent == tree);
+    DISSECTOR_ASSERT(fixed_item->parent == tree);
 
     /*** cut item_to_move out ***/
 
@@ -3382,7 +3394,10 @@ proto_tree_move_item(proto_tree *tree, proto_item *fixed_item, proto_item *item_
     if(tree->first_child == item_to_move) {
         /* simply change first child to next */
         tree->first_child = item_to_move->next;
+
+        DISSECTOR_ASSERT(tree->last_child != item_to_move);
     } else {
+        proto_item *curr_item;
         /* find previous and change it's next */
         for(curr_item = tree->first_child; curr_item != NULL; curr_item = curr_item->next) {
             if(curr_item->next == item_to_move) {

diff --git a/epan/proto.h b/epan/proto.h
index 1a48bd2732a48749cc1d5fd107fd269d39505e2e..65da12ffc21ffe7ccbd99fac5ffd8bf60c8ef729 100644 (file)
--- a/epan/proto.h
+++ b/epan/proto.h
@@ -334,7 +334,7 @@ typedef proto_node proto_item;
 typedef void (*proto_tree_foreach_func)(proto_node *, gpointer);
 typedef gboolean (*proto_tree_traverse_func)(proto_node *, gpointer);
 
-extern gboolean proto_tree_traverse_in_order(proto_tree *tree,
+extern gboolean proto_tree_traverse_post_order(proto_tree *tree,
     proto_tree_traverse_func func, gpointer data);
 extern void proto_tree_children_foreach(proto_tree *tree,
     proto_tree_foreach_func func, gpointer data);