From c509cd07008b1f9b3402eeb7d7f3f6d504aa4fdb Mon Sep 17 00:00:00 2001 From: "Gao,Yan" Date: Fri, 10 Feb 2017 15:44:16 +0100 Subject: [PATCH 1/2] Fix: libcrmcommon: Correctly compare XML comments to prevent crmd from getting into infinite election loop With b7fa323, crmd could still get into an infinite election loop when there was more than one comment with the exactly same text at the same level within a CIB XML element. For example: ''' ''' Basically, it'd produce big messes if using the diff operation "move" for XML comments in such a case. With this commit, it strictly tries to match the XML comments at the exactly same offsets when comparing v2 patchset, so that only the diff operations "create" and "delete" will be used for XML comments. --- lib/common/xml.c | 46 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index 65237c8..fd80fe1 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -98,7 +98,7 @@ static filter_t filter[] = { /* *INDENT-ON* */ static xmlNode *subtract_xml_comment(xmlNode * parent, xmlNode * left, xmlNode * right, gboolean * changed); -static xmlNode *find_xml_comment(xmlNode * root, xmlNode * search_comment); +static xmlNode *find_xml_comment(xmlNode * root, xmlNode * search_comment, gboolean exact); static int add_xml_comment(xmlNode * parent, xmlNode * target, xmlNode * update); static bool __xml_acl_check(xmlNode *xml, const char *name, enum xml_private_flags mode); const char *__xml_acl_to_text(enum xml_private_flags flags); @@ -1555,11 +1555,11 @@ xml_accept_changes(xmlNode * xml) } static xmlNode * -find_element(xmlNode *haystack, xmlNode *needle) +find_element(xmlNode *haystack, xmlNode *needle, gboolean exact) { CRM_CHECK(needle != NULL, return NULL); return (needle->type == XML_COMMENT_NODE)? - find_xml_comment(haystack, needle) + find_xml_comment(haystack, needle, exact) : find_entity(haystack, crm_element_name(needle), ID(needle)); } @@ -1615,7 +1615,7 @@ __subtract_xml_object(xmlNode * target, xmlNode * patch) xmlNode *target_child = cIter; cIter = __xml_next(cIter); - patch_child = find_element(patch, target_child); + patch_child = find_element(patch, target_child, FALSE); __subtract_xml_object(target_child, patch_child); } free(id); @@ -1677,7 +1677,7 @@ __add_xml_object(xmlNode * parent, xmlNode * target, xmlNode * patch) for (patch_child = __xml_first_child(patch); patch_child != NULL; patch_child = __xml_next(patch_child)) { - target_child = find_element(target, patch_child); + target_child = find_element(target, patch_child, FALSE); __add_xml_object(target, target_child, patch_child); } } @@ -4026,7 +4026,7 @@ __xml_diff_object(xmlNode * old, xmlNode * new) for (cIter = __xml_first_child(old); cIter != NULL; ) { xmlNode *old_child = cIter; - xmlNode *new_child = find_element(new, cIter); + xmlNode *new_child = find_element(new, cIter, TRUE); cIter = __xml_next(cIter); if(new_child) { @@ -4041,7 +4041,7 @@ __xml_diff_object(xmlNode * old, xmlNode * new) __xml_acl_apply(top); /* Make sure any ACLs are applied to 'candidate' */ free_xml(candidate); - if (find_element(new, old_child) == NULL) { + if (find_element(new, old_child, TRUE) == NULL) { xml_private_t *p = old_child->_private; p->flags |= xpf_skip; @@ -4051,7 +4051,7 @@ __xml_diff_object(xmlNode * old, xmlNode * new) for (cIter = __xml_first_child(new); cIter != NULL; ) { xmlNode *new_child = cIter; - xmlNode *old_child = find_element(old, cIter); + xmlNode *old_child = find_element(old, cIter, TRUE); cIter = __xml_next(cIter); if(old_child == NULL) { @@ -4226,18 +4226,36 @@ in_upper_context(int depth, int context, xmlNode * xml_node) } static xmlNode * -find_xml_comment(xmlNode * root, xmlNode * search_comment) +find_xml_comment(xmlNode * root, xmlNode * search_comment, gboolean exact) { xmlNode *a_child = NULL; + int search_offset = __xml_offset(search_comment); CRM_CHECK(search_comment->type == XML_COMMENT_NODE, return NULL); for (a_child = __xml_first_child(root); a_child != NULL; a_child = __xml_next(a_child)) { - if (a_child->type != XML_COMMENT_NODE) { - continue; + if (exact) { + int offset = __xml_offset(a_child); + xml_private_t *p = a_child->_private; + + if (offset < search_offset) { + continue; + + } else if (offset > search_offset) { + return NULL; + } + + if (is_set(p->flags, xpf_skip)) { + continue; + } } - if (safe_str_eq((const char *)a_child->content, (const char *)search_comment->content)) { + + if (a_child->type == XML_COMMENT_NODE + && safe_str_eq((const char *)a_child->content, (const char *)search_comment->content)) { return a_child; + + } else if (exact) { + return NULL; } } @@ -4332,7 +4350,7 @@ subtract_xml_object(xmlNode * parent, xmlNode * left, xmlNode * right, left_child = __xml_next(left_child)) { gboolean child_changed = FALSE; - right_child = find_element(right, left_child); + right_child = find_element(right, left_child, FALSE); subtract_xml_object(diff, left_child, right_child, full, &child_changed, marker); if (child_changed) { *changed = TRUE; @@ -4458,7 +4476,7 @@ add_xml_comment(xmlNode * parent, xmlNode * target, xmlNode * update) CRM_CHECK(update->type == XML_COMMENT_NODE, return 0); if (target == NULL) { - target = find_xml_comment(parent, update); + target = find_xml_comment(parent, update, FALSE); } if (target == NULL) { -- 1.8.3.1 From 362f02874b6be46bf2648fb45ebc6bc636d48965 Mon Sep 17 00:00:00 2001 From: "Gao,Yan" Date: Mon, 13 Feb 2017 17:47:23 +0100 Subject: [PATCH 2/2] Fix: libcrmcommon: Correctly delete XML comments according to their positions Previously, v2 patchset was not able to define which exact XML comments were expected to be deleted in a CIB XML element. It was a problem if there was more than one comment at the same level within a CIB XML element. This commit resolves it by adding and handling a "position" field in the diff operation "delete" for XML comments. --- lib/common/xml.c | 121 +++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 100 insertions(+), 21 deletions(-) diff --git a/lib/common/xml.c b/lib/common/xml.c index fd80fe1..6dce4cb 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -78,7 +78,7 @@ typedef struct xml_private_s uint32_t flags; char *user; GListPtr acls; - GListPtr deleted_paths; + GListPtr deleted_objs; } xml_private_t; typedef struct xml_acl_s { @@ -86,6 +86,11 @@ typedef struct xml_acl_s { char *xpath; } xml_acl_t; +typedef struct xml_deleted_obj_s { + char *path; + int position; +} xml_deleted_obj_t; + /* *INDENT-OFF* */ static filter_t filter[] = { @@ -275,6 +280,17 @@ __xml_acl_free(void *data) } static void +__xml_deleted_obj_free(void *data) +{ + if(data) { + xml_deleted_obj_t *deleted_obj = data; + + free(deleted_obj->path); + free(deleted_obj); + } +} + +static void __xml_private_clean(xml_private_t *p) { if(p) { @@ -288,9 +304,9 @@ __xml_private_clean(xml_private_t *p) p->acls = NULL; } - if(p->deleted_paths) { - g_list_free_full(p->deleted_paths, free); - p->deleted_paths = NULL; + if(p->deleted_objs) { + g_list_free_full(p->deleted_objs, __xml_deleted_obj_free); + p->deleted_objs = NULL; } } } @@ -1094,10 +1110,10 @@ is_config_change(xmlNode *xml) if(xml->doc && xml->doc->_private) { p = xml->doc->_private; - for(gIter = p->deleted_paths; gIter; gIter = gIter->next) { - char *path = gIter->data; + for(gIter = p->deleted_objs; gIter; gIter = gIter->next) { + xml_deleted_obj_t *deleted_obj = gIter->data; - if(strstr(path, "/"XML_TAG_CIB"/"XML_CIB_TAG_CONFIGURATION) != NULL) { + if(strstr(deleted_obj->path, "/"XML_TAG_CIB"/"XML_CIB_TAG_CONFIGURATION) != NULL) { return TRUE; } } @@ -1241,11 +1257,15 @@ xml_create_patchset_v2(xmlNode *source, xmlNode *target) crm_xml_add(v, vfields[lpc], value); } - for(gIter = doc->deleted_paths; gIter; gIter = gIter->next) { + for(gIter = doc->deleted_objs; gIter; gIter = gIter->next) { + xml_deleted_obj_t *deleted_obj = gIter->data; xmlNode *change = create_xml_node(patchset, XML_DIFF_CHANGE); crm_xml_add(change, XML_DIFF_OP, "delete"); - crm_xml_add(change, XML_DIFF_PATH, gIter->data); + crm_xml_add(change, XML_DIFF_PATH, deleted_obj->path); + if (deleted_obj->position >= 0) { + crm_xml_add_int(change, XML_DIFF_POSITION, deleted_obj->position); + } } __xml_build_changes(target, patchset); @@ -1473,7 +1493,15 @@ xml_log_patchset(uint8_t log_level, const char *function, xmlNode * patchset) } } else if(strcmp(op, "delete") == 0) { - do_crm_log_alias(log_level, __FILE__, function, __LINE__, "-- %s", xpath); + int position = -1; + + crm_element_value_int(change, XML_DIFF_POSITION, &position); + if (position >= 0) { + do_crm_log_alias(log_level, __FILE__, function, __LINE__, "-- %s (%d)", xpath, position); + + } else { + do_crm_log_alias(log_level, __FILE__, function, __LINE__, "-- %s", xpath); + } } } return; @@ -1521,8 +1549,17 @@ xml_log_changes(uint8_t log_level, const char *function, xmlNode * xml) return; } - for(gIter = doc->deleted_paths; gIter; gIter = gIter->next) { - do_crm_log_alias(log_level, __FILE__, function, __LINE__, "-- %s", (char*)gIter->data); + for(gIter = doc->deleted_objs; gIter; gIter = gIter->next) { + xml_deleted_obj_t *deleted_obj = gIter->data; + + if (deleted_obj->position >= 0) { + do_crm_log_alias(log_level, __FILE__, function, __LINE__, "-- %s (%d)", + deleted_obj->path, deleted_obj->position); + + } else { + do_crm_log_alias(log_level, __FILE__, function, __LINE__, "-- %s", + deleted_obj->path); + } } log_data_element(log_level, __FILE__, function, __LINE__, "+ ", xml, 0, @@ -1881,7 +1918,7 @@ xml_apply_patchset_v1(xmlNode *xml, xmlNode *patchset, bool check_version) } static xmlNode * -__first_xml_child_match(xmlNode *parent, const char *name, const char *id) +__first_xml_child_match(xmlNode *parent, const char *name, const char *id, int position) { xmlNode *cIter = NULL; @@ -1894,13 +1931,21 @@ __first_xml_child_match(xmlNode *parent, const char *name, const char *id) continue; } } + + /* The "position" makes sense only for XML comments for now */ + if (cIter->type == XML_COMMENT_NODE + && position >= 0 + && __xml_offset(cIter) != position) { + continue; + } + return cIter; } return NULL; } static xmlNode * -__xml_find_path(xmlNode *top, const char *key) +__xml_find_path(xmlNode *top, const char *key, int target_position) { xmlNode *target = (xmlNode*)top->doc; char *id = malloc(XML_BUFFER_SIZE); @@ -1923,13 +1968,19 @@ __xml_find_path(xmlNode *top, const char *key) } else if(tag && section) { int f = sscanf (section, "%[^[][@id='%[^']", tag, id); + int current_position = -1; + + /* The "target_position" is for the target tag */ + if (rc == 1 && target_position >= 0) { + current_position = target_position; + } switch(f) { case 1: - target = __first_xml_child_match(target, tag, NULL); + target = __first_xml_child_match(target, tag, NULL, current_position); break; case 2: - target = __first_xml_child_match(target, tag, id); + target = __first_xml_child_match(target, tag, id, current_position); break; default: crm_trace("Aborting on %s", section); @@ -1975,16 +2026,20 @@ xml_apply_patchset_v2(xmlNode *xml, xmlNode *patchset, bool check_version) xmlNode *match = NULL; const char *op = crm_element_value(change, XML_DIFF_OP); const char *xpath = crm_element_value(change, XML_DIFF_PATH); + int position = -1; crm_trace("Processing %s %s", change->name, op); if(op == NULL) { continue; } + if(strcmp(op, "delete") == 0) { + crm_element_value_int(change, XML_DIFF_POSITION, &position); + } #if 0 match = get_xpath_object(xpath, xml, LOG_TRACE); #else - match = __xml_find_path(xml, xpath); + match = __xml_find_path(xml, xpath, position); #endif crm_trace("Performing %s on %s with %p", op, xpath, match); @@ -2583,8 +2638,8 @@ xml_get_path(xmlNode *xml) return NULL; } -void -free_xml(xmlNode * child) +static void +free_xml_with_position(xmlNode * child, int position) { if (child != NULL) { xmlNode *top = NULL; @@ -2613,9 +2668,25 @@ free_xml(xmlNode * child) char buffer[XML_BUFFER_SIZE]; if(__get_prefix(NULL, child, buffer, offset) > 0) { + xml_deleted_obj_t *deleted_obj = calloc(1, sizeof(xml_deleted_obj_t)); + crm_trace("Deleting %s %p from %p", buffer, child, doc); + + deleted_obj->path = strdup(buffer); + + deleted_obj->position = -1; + /* Record the "position" only for XML comments for now */ + if (child->type == XML_COMMENT_NODE) { + if (position >= 0) { + deleted_obj->position = position; + + } else { + deleted_obj->position = __xml_offset(child); + } + } + p = doc->_private; - p->deleted_paths = g_list_append(p->deleted_paths, strdup(buffer)); + p->deleted_objs = g_list_append(p->deleted_objs, deleted_obj); set_doc_flag(child, xpf_dirty); } } @@ -2629,6 +2700,13 @@ free_xml(xmlNode * child) } } + +void +free_xml(xmlNode * child) +{ + free_xml_with_position(child, -1); +} + xmlNode * copy_xml(xmlNode * src) { @@ -4039,7 +4117,8 @@ __xml_diff_object(xmlNode * old, xmlNode * new) __xml_node_clean(candidate); __xml_acl_apply(top); /* Make sure any ACLs are applied to 'candidate' */ - free_xml(candidate); + /* Record the old position */ + free_xml_with_position(candidate, __xml_offset(old_child)); if (find_element(new, old_child, TRUE) == NULL) { xml_private_t *p = old_child->_private; -- 1.8.3.1