From eb820a57a66fbc0ab53be6379d5ffa37431c7d8c Mon Sep 17 00:00:00 2001 From: Kalle Olavi Niemitalo Date: Sun, 15 Feb 2009 03:10:14 +0200 Subject: [PATCH 1/3] 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 *); From d14f65a331a980e1bc4666b8c52b7b61ccd38f58 Mon Sep 17 00:00:00 2001 From: Kalle Olavi Niemitalo Date: Sun, 15 Feb 2009 04:27:39 +0200 Subject: [PATCH 2/3] bug 1067: Comments about freeing the DOM document node. --- src/document/dom/renderer.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/document/dom/renderer.c b/src/document/dom/renderer.c index 35ac4810..38cd0830 100644 --- a/src/document/dom/renderer.c +++ b/src/document/dom/renderer.c @@ -720,6 +720,10 @@ render_dom_document_end(struct dom_stack *stack, struct dom_node *node, void *da render_dom_flush(renderer, renderer->end); } + /* It is not necessary to return DOM_CODE_FREE_NODE here. + * Because the parser was created with the SGML_PARSER_STREAM + * type, the stack has the DOM_STACK_FLAG_FREE_NODES flag and + * implicitly frees all nodes popped from it. */ return DOM_CODE_OK; } @@ -947,6 +951,10 @@ dom_rss_pop_document(struct dom_stack *stack, struct dom_node *root, void *data) done_dom_string(&renderer->text); mem_free_if(renderer->items); + /* ELinks does not provide any sort of DOM access to the RSS + * document after it has been rendered. Tell the caller to + * free the document node and all of its children. Otherwise, + * they would leak. */ return DOM_CODE_FREE_NODE; } From c8cee1df61638631738cc8908b962bd4094cc52e Mon Sep 17 00:00:00 2001 From: Kalle Olavi Niemitalo Date: Sun, 15 Feb 2009 05:02:43 +0200 Subject: [PATCH 3/3] bug 1067: More verbose NEWS. --- NEWS | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index b47b0eba..144c51cc 100644 --- a/NEWS +++ b/NEWS @@ -34,7 +34,12 @@ Other changes: * minor bug 761: When reading bookmarks from an XBEL file, distinguish attribute names from attribute values. * enhancement: Updated ISO 8859-7, ISO 8859-16, KOI8-R, and MacRoman. -* critical: bug 1067: crash in call_dom_stack_callbacks with RSS + +Bugs that should be removed from NEWS before the 0.12.0 release: + +* critical bug 1067: Fixed a crash in the RSS parser that ``configure + --enable-html-highlight'' enables. ELinks 0.12pre1 was the first + release that supported RSS at all. ELinks 0.12pre2: ----------------