From eb820a57a66fbc0ab53be6379d5ffa37431c7d8c Mon Sep 17 00:00:00 2001
From: Kalle Olavi Niemitalo <kon@iki.fi>
Date: Sun, 15 Feb 2009 03:10:14 +0200
Subject: [PATCH] bug 1067: Assertions and comments about done_dom_node().

In bug 1067, dom_rss_pop_document() freed a node with done_dom_node()
even though call_dom_node_callbacks() was still using that node.  This
made call_dom_node_callbacks() read a function pointer from beyond the
end of an array and call that.  Add assertions to detect out-of-range
node types, and comments to warn about the bug.
---
 src/dom/node.c  | 13 +++++++++++++
 src/dom/node.h  |  4 +++-
 src/dom/stack.c | 11 +++++++++++
 src/dom/stack.h |  8 +++++++-
 4 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/src/dom/node.c b/src/dom/node.c
index f77a8d78..0aaa309c 100644
--- a/src/dom/node.c
+++ b/src/dom/node.c
@@ -392,6 +392,7 @@ done_dom_node_data(struct dom_node *node)
 	union dom_node_data *data;
 
 	assert(node);
+	assert(node->type < DOM_NODES); /* unsigned comparison */
 
 	data = &node->data;
 
@@ -427,6 +428,18 @@ done_dom_node_data(struct dom_node *node)
 
 	if (node->allocated)
 		done_dom_string(&node->string);
+
+	/* call_dom_stack_callbacks() asserts that the node type is
+	 * within range.  If assertions are enabled, store an
+	 * out-of-range value there to make the assertion more likely
+	 * to fail if a callback has freed the node prematurely.
+	 * This is not entirely reliable though, because the memory
+	 * is freed below and may be allocated for some other purpose
+	 * before the assertion.  */
+#ifndef CONFIG_FASTMEM
+	node->type = -1;
+#endif
+
 	mem_free(node);
 }
 
diff --git a/src/dom/node.h b/src/dom/node.h
index 40dfd855..5ab492df 100644
--- a/src/dom/node.h
+++ b/src/dom/node.h
@@ -314,7 +314,9 @@ get_dom_node_map_entry(struct dom_node_list *node_map,
 		       enum dom_node_type type, uint16_t subtype,
 		       struct dom_string *name);
 
-/* Removes the node and all its children and free()s itself */
+/* Removes the node and all its children and free()s itself.
+ * A dom_stack_callback_T must not use this to free the node
+ * it gets as a parameter.  */
 void done_dom_node(struct dom_node *node);
 
 #ifndef DEBUG_MEMLEAK
diff --git a/src/dom/stack.c b/src/dom/stack.c
index 4b750454..a55548f1 100644
--- a/src/dom/stack.c
+++ b/src/dom/stack.c
@@ -139,6 +139,17 @@ call_dom_stack_callbacks(struct dom_stack *stack, struct dom_stack_state *state,
 		struct dom_stack_context *context = stack->contexts[i];
 		dom_stack_callback_T callback;
 
+		assert(state->node->type < DOM_NODES); /* unsigned comparison */
+		if_assert_failed {
+			/* The node type is out of range for the
+			 * callback arrays.  The node may have been
+			 * corrupted or already freed.  Ignore
+			 * free_node here because attempting to free
+			 * the node would probably just corrupt things
+			 * further.  */
+			return 0;
+		}
+
 		if (action == DOM_STACK_PUSH)
 			callback = context->info->push[state->node->type];
 		else
diff --git a/src/dom/stack.h b/src/dom/stack.h
index 423c5bb5..3c230ef4 100644
--- a/src/dom/stack.h
+++ b/src/dom/stack.h
@@ -51,7 +51,13 @@ struct dom_stack;
 
 /** DOM stack callback
  *
- * Used by contexts, for 'hooking' into the node traversing. */
+ * Used by contexts, for 'hooking' into the node traversing.
+ *
+ * This callback must not call done_dom_node() to free the node it
+ * gets as a parameter, because call_dom_node_callbacks() may be
+ * intending to call more callbacks for the same node.  Instead, the
+ * callback can return ::DOM_CODE_FREE_NODE, and the node will then
+ * be freed after the callbacks of all contexts have been called.  */
 typedef enum dom_code
 	(*dom_stack_callback_T)(struct dom_stack *, struct dom_node *, void *);