From c509cd07008b1f9b3402eeb7d7f3f6d504aa4fdb Mon Sep 17 00:00:00 2001

From: "Gao,Yan" <ygao@suse.com>

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" <ygao@suse.com>

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