Skip to content
Browse files

Fix: xml: Prevent use-after-free when not processing all xpath query …

…results
  • Loading branch information...
1 parent 0ea0df2 commit fe34328772f9d235e2aca8a07788f7b658510a58 @beekhof committed Mar 20, 2013
Showing with 85 additions and 68 deletions.
  1. +17 −18 crmd/te_callbacks.c
  2. +4 −4 fencing/commands.c
  3. +6 −6 fencing/main.c
  4. +7 −6 include/crm/common/xml.h
  5. +1 −1 lib/cib/cib_acl.c
  6. +3 −3 lib/cib/cib_ops.c
  7. +1 −1 lib/cib/cib_utils.c
  8. +41 −24 lib/common/xml.c
  9. +4 −4 lib/fencing/st_client.c
  10. +1 −1 tools/crm_mon.c
View
35 crmd/te_callbacks.c
@@ -1,16 +1,16 @@
-/*
+/*
* Copyright (C) 2004 Andrew Beekhof <andrew@beekhof.net>
- *
+ *
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public
* License as published by the Free Software Foundation; either
* version 2 of the License, or (at your option) any later version.
- *
+ *
* This software is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* General Public License for more details.
- *
+ *
* You should have received a copy of the GNU General Public
* License along with this library; if not, write to the Free Software
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
@@ -137,7 +137,7 @@ te_update_diff(const char *event, xmlNode * msg)
goto bail;
} else if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
/* Tickets Attributes - Removed */
@@ -151,7 +151,7 @@ te_update_diff(const char *event, xmlNode * msg)
goto bail;
} else if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
/* Transient Attributes - Added/Updated */
@@ -177,11 +177,10 @@ te_update_diff(const char *event, xmlNode * msg)
goto bail;
}
}
+ }
- xmlXPathFreeObject(xpathObj);
-
- } else if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ if (xpathObj) {
+ freeXpathObject(xpathObj);
}
/* Transient Attributes - Removed */
@@ -196,15 +195,15 @@ te_update_diff(const char *event, xmlNode * msg)
goto bail;
} else if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
/*
* Check for and fast-track the processing of LRM refreshes
* In large clusters this can result in _huge_ speedups
*
* Unfortunately we can only do so when there are no pending actions
- * Otherwise we could miss updates we're waiting for and stall
+ * Otherwise we could miss updates we're waiting for and stall
*
*/
xpathObj = NULL;
@@ -228,7 +227,7 @@ te_update_diff(const char *event, xmlNode * msg)
abort_transition(INFINITY, tg_restart, "LRM Refresh", NULL);
goto bail;
}
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
/* Process operation updates */
@@ -237,7 +236,7 @@ te_update_diff(const char *event, xmlNode * msg)
"//" F_CIB_UPDATE_RESULT "//" XML_TAG_DIFF_ADDED "//" XML_LRM_TAG_RSC_OP);
if (xpathObj) {
process_resource_updates(xpathObj);
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
/* Detect deleted (as opposed to replaced or added) actions - eg. crm_resource -C */
@@ -271,7 +270,7 @@ te_update_diff(const char *event, xmlNode * msg)
node);
abort_transition(INFINITY, tg_restart, "Resource op removal", match);
if (op_match) {
- xmlXPathFreeObject(op_match);
+ freeXpathObject(op_match);
}
free(rsc_op_xpath);
goto bail;
@@ -283,15 +282,15 @@ te_update_diff(const char *event, xmlNode * msg)
}
if (op_match) {
- xmlXPathFreeObject(op_match);
+ freeXpathObject(op_match);
}
free(rsc_op_xpath);
}
}
bail:
if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
}
@@ -327,7 +326,7 @@ process_te_message(xmlNode * msg, xmlNode * xml_data)
xpathObj = xpath_search(xml_data, "//" XML_LRM_TAG_RSC_OP);
if (xpathObj) {
process_resource_updates(xpathObj);
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
xpathObj = NULL;
} else {
View
8 fencing/commands.c
@@ -527,13 +527,13 @@ is_nodeid_required(xmlNode * xml)
xpath = xpath_search(xml, "//parameter[@name='nodeid']");
if (!xpath || xpath->nodesetval->nodeNr <= 0) {
if (xpath) {
- xmlXPathFreeObject(xpath);
+ freeXpathObject(xpath);
}
return FALSE;
}
if (xpath) {
- xmlXPathFreeObject(xpath);
+ freeXpathObject(xpath);
}
return TRUE;
@@ -555,7 +555,7 @@ get_on_target_actions(xmlNode * xml)
if (!xpath || !xpath->nodesetval) {
if (xpath) {
- xmlXPathFreeObject(xpath);
+ freeXpathObject(xpath);
}
return NULL;
}
@@ -583,7 +583,7 @@ get_on_target_actions(xmlNode * xml)
}
if (xpath) {
- xmlXPathFreeObject(xpath);
+ freeXpathObject(xpath);
}
if (!strlen(actions)) {
View
12 fencing/main.c
@@ -576,7 +576,7 @@ fencing_topology_init(xmlNode * msg)
register_fencing_topology(xpathObj, TRUE);
if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
}
@@ -698,15 +698,15 @@ update_cib_stonith_devices(const char *event, xmlNode * msg)
if (xpath_obj && xpath_obj->nodesetval->nodeNr > 0) {
/* Safest and simplest to always recompute */
needs_update = TRUE;
- xmlXPathFreeObject(xpath_obj);
+ freeXpathObject(xpath_obj);
reason = "new location constraint";
}
/* process deletions */
xpath_obj = xpath_search(msg, "//" F_CIB_UPDATE_RESULT "//" XML_TAG_DIFF_REMOVED "//" XML_CIB_TAG_RESOURCE);
if (xpath_obj && xpath_obj->nodesetval->nodeNr > 0) {
remove_cib_device(xpath_obj);
- xmlXPathFreeObject(xpath_obj);
+ freeXpathObject(xpath_obj);
}
/* process additions */
@@ -722,7 +722,7 @@ update_cib_stonith_devices(const char *event, xmlNode * msg)
crm_info("resource[%d] = %s", lpc, path);
}
- xmlXPathFreeObject(xpath_obj);
+ freeXpathObject(xpath_obj);
reason = "new resource";
}
@@ -745,7 +745,7 @@ update_fencing_topology(const char *event, xmlNode * msg)
remove_fencing_topology(xpathObj);
if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
/* Process additions and changes */
@@ -755,7 +755,7 @@ update_fencing_topology(const char *event, xmlNode * msg)
register_fencing_topology(xpathObj, FALSE);
if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
}
static bool have_cib_devices = FALSE;
View
13 include/crm/common/xml.h
@@ -1,16 +1,16 @@
-/*
+/*
* Copyright (C) 2004 Andrew Beekhof <andrew@beekhof.net>
- *
+ *
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public
* License as published by the Free Software Foundation; either
* version 2 of the License, or (at your option) any later version.
- *
+ *
* This software is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* General Public License for more details.
- *
+ *
* You should have received a copy of the GNU General Public
* License along with this library; if not, write to the Free Software
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
@@ -68,7 +68,7 @@ xmlDoc *getDocPtr(xmlNode * node);
*
* Copy all the attributes/properties from src into target.
*
- * Not recursive, does not return anything.
+ * Not recursive, does not return anything.
*
*/
void copy_in_properties(xmlNode * target, xmlNode * src);
@@ -108,7 +108,7 @@ const char *crm_xml_add_int(xmlNode * node, const char *name, int value);
void unlink_xml_node(xmlNode * node);
/*
- *
+ *
*/
void purge_diff_markers(xmlNode * a_node);
@@ -238,6 +238,7 @@ xmlXPathObjectPtr xpath_search(xmlNode * xml_top, const char *path);
gboolean cli_config_update(xmlNode ** xml, int *best_version, gboolean to_logs);
xmlNode *expand_idref(xmlNode * input, xmlNode * top);
+void freeXpathObject(xmlXPathObjectPtr xpathObj);
xmlNode *getXpathResult(xmlXPathObjectPtr xpathObj, int index);
#endif
View
2 lib/cib/cib_acl.c
@@ -546,7 +546,7 @@ search_xpath_objects(GListPtr * objects, xmlNode * xml_obj, const char *xpath)
}
if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
return match_found;
}
View
6 lib/cib/cib_ops.c
@@ -794,7 +794,7 @@ cib_config_changed(xmlNode * last, xmlNode * next, xmlNode ** diff)
goto done;
} else if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
/*
@@ -839,7 +839,7 @@ cib_config_changed(xmlNode * last, xmlNode * next, xmlNode ** diff)
done:
if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
return config_changes;
}
@@ -992,7 +992,7 @@ cib_process_xpath(const char *op, int options, const char *section, xmlNode * re
}
if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
return rc;
View
2 lib/cib/cib_utils.c
@@ -886,7 +886,7 @@ cib_internal_config_changed(xmlNode * diff)
}
if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
return changed;
View
65 lib/common/xml.c
@@ -2873,6 +2873,46 @@ update_validation(xmlNode ** xml_blob, int *best, gboolean transform, gboolean t
return rc;
}
+/*
+ * From xpath2.c
+ *
+ * All the elements returned by an XPath query are pointers to
+ * elements from the tree *except* namespace nodes where the XPath
+ * semantic is different from the implementation in libxml2 tree.
+ * As a result when a returned node set is freed when
+ * xmlXPathFreeObject() is called, that routine must check the
+ * element type. But node from the returned set may have been removed
+ * by xmlNodeSetContent() resulting in access to freed data.
+ *
+ * This can be exercised by running
+ * valgrind xpath2 test3.xml '//discarded' discarded
+ *
+ * There is 2 ways around it:
+ * - make a copy of the pointers to the nodes from the result set
+ * then call xmlXPathFreeObject() and then modify the nodes
+ * or
+ * - remove the references from the node set, if they are not
+ namespace nodes, before calling xmlXPathFreeObject().
+ */
+void
+freeXpathObject(xmlXPathObjectPtr xpathObj)
+{
+ int lpc;
+
+ if(xpathObj == NULL || xpathObj->nodesetval == NULL) {
+ return;
+ }
+
+ for(lpc = 0; lpc < xpathObj->nodesetval->nodeNr; lpc++) {
+ if (xpathObj->nodesetval->nodeTab[lpc]->type != XML_NAMESPACE_DECL) {
+ xpathObj->nodesetval->nodeTab[lpc] = NULL;
+ }
+ }
+
+ /* _Now_ its safe to free it */
+ xmlXPathFreeObject(xpathObj);
+}
+
xmlNode *
getXpathResult(xmlXPathObjectPtr xpathObj, int index)
{
@@ -2889,29 +2929,6 @@ getXpathResult(xmlXPathObjectPtr xpathObj, int index)
match = xpathObj->nodesetval->nodeTab[index];
CRM_CHECK(match != NULL, return NULL);
- /*
- * From xpath2.c
- *
- * All the elements returned by an XPath query are pointers to
- * elements from the tree *except* namespace nodes where the XPath
- * semantic is different from the implementation in libxml2 tree.
- * As a result when a returned node set is freed when
- * xmlXPathFreeObject() is called, that routine must check the
- * element type. But node from the returned set may have been removed
- * by xmlNodeSetContent() resulting in access to freed data.
- * This can be exercised by running
- * valgrind xpath2 test3.xml '//discarded' discarded
- * There is 2 ways around it:
- * - make a copy of the pointers to the nodes from the result set
- * then call xmlXPathFreeObject() and then modify the nodes
- * or
- * - remove the reference to the modified nodes from the node set
- * as they are processed, if they are not namespace nodes.
- */
- if (xpathObj->nodesetval->nodeTab[index]->type != XML_NAMESPACE_DECL) {
- xpathObj->nodesetval->nodeTab[index] = NULL;
- }
-
if (match->type == XML_DOCUMENT_NODE) {
/* Will happen if section = '/' */
match = match->children;
@@ -3126,7 +3143,7 @@ get_xpath_object(const char *xpath, xmlNode * xml_obj, int error_level)
}
if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
free(nodePath);
View
8 lib/fencing/st_client.c
@@ -1080,7 +1080,7 @@ stonith_api_device_metadata(stonith_t * stonith, int call_options, const char *a
}
if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
/* Now fudge the metadata so that the start/stop actions appear */
@@ -1098,7 +1098,7 @@ stonith_api_device_metadata(stonith_t * stonith, int call_options, const char *a
}
if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
/* Now fudge the metadata so that the port isn't required in the configuration */
@@ -1111,7 +1111,7 @@ stonith_api_device_metadata(stonith_t * stonith, int call_options, const char *a
}
if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
free(buffer);
buffer = dump_xml_formatted(xml);
@@ -1252,7 +1252,7 @@ stonith_api_query(stonith_t * stonith, int call_options, const char *target,
*devices = stonith_key_value_add(*devices, NULL, crm_element_value(match, XML_ATTR_ID));
}
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
free_xml(output);
View
2 tools/crm_mon.c
@@ -2169,7 +2169,7 @@ crm_diff_update(const char *event, xmlNode * msg)
}
}
if (xpathObj) {
- xmlXPathFreeObject(xpathObj);
+ freeXpathObject(xpathObj);
}
}

0 comments on commit fe34328

Please sign in to comment.
Something went wrong with that request. Please try again.