replmd: Make replmd_process_linked_attribute() mem dependencies clearer

This patch should not alter functionality - it is just making memory
assumptions used in replmd_process_linked_attribute() clearer.

When adding/removing msg->elements we have to take care, as this will
invalidate things like the parsed-DN array or old ldb_message_element
pointers. This has always been the case (i.e. f6bc4c08b19f5615a49),
however, now we need to take even more care, as the msg being modified
is re-used and split across 2 different functions.

Add more code comments to highlight this. We can also free
pdn_list/old_el to prevent them being incorrectly used after realloc.
It seems appropriate to also add a sanity-check that the tmp_ctx alloc
succeeds (which all the other memory hangs off).

Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Autobuild-User(master): Tim Beale <timbeale@samba.org>
Autobuild-Date(master): Wed Nov 21 05:31:10 CET 2018 on sn-devel-144

diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index b88aaf251e7604e4b52b65286e3661ac34470063..d89776e4bc633b5a370a71e411895f4f04cc2659 100644 (file)
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7937,8 +7937,21 @@ static int replmd_check_singleval_la_conflict(struct ldb_module *module,
        return LDB_SUCCESS;
 }
 
-/*
-  process one linked attribute structure
+/**
+ * Processes one linked attribute received via replication.
+ * @param msg the source object for the link. For optimization, the same msg
+ * can be used across multiple calls to replmd_process_linked_attribute().
+ * @note this function should not add or remove any msg attributes (it should
+ * only add/modify values for the linked attribute being processed). Otherwise
+ * msg->elements is realloc'd and old_el/pdn_list pointers will be invalidated
+ * @param attr schema info for the linked attribute
+ * @param la_entry the linked attribute info received via DRS
+ * @param old_el the corresponding msg->element[] for the linked attribute
+ * @param pdn_list a (binary-searchable) parsed DN array for the existing link
+ * values in the msg. E.g. for a group, this is the existing members.
+ * @param change what got modified: either nothing, an existing link value was
+ * modified, or a new link value was added.
+ * @returns LDB_SUCCESS if OK, an error otherwise
  */
 static int replmd_process_linked_attribute(struct ldb_module *module,
                                           TALLOC_CTX *mem_ctx,
@@ -8181,7 +8194,7 @@ static int replmd_start_transaction(struct ldb_module *module)
 
 /**
  * Processes a group of linked attributes that apply to the same source-object
- * and attribute-ID
+ * and attribute-ID (and were received in the same replication chunk).
  */
 static int replmd_process_la_group(struct ldb_module *module,
                                   struct replmd_private *replmd_private,
@@ -8190,7 +8203,7 @@ static int replmd_process_la_group(struct ldb_module *module,
        struct la_entry *la = NULL;
        struct la_entry *prev = NULL;
        int ret;
-       TALLOC_CTX *tmp_ctx = talloc_new(la_group);
+       TALLOC_CTX *tmp_ctx = NULL;
        struct la_entry *first_la = DLIST_TAIL(la_group->la_entries);
        struct ldb_message *msg = NULL;
        enum deletion_state deletion_state = OBJECT_NOT_DELETED;
@@ -8203,6 +8216,11 @@ static int replmd_process_la_group(struct ldb_module *module,
        time_t t;
        uint64_t seq_num = 0;
 
+       tmp_ctx = talloc_new(la_group);
+       if (tmp_ctx == NULL) {
+               return ldb_oom(ldb);
+       }
+
        /*
         * get the attribute being modified and the search result for the
         * source object
@@ -8248,6 +8266,7 @@ static int replmd_process_la_group(struct ldb_module *module,
        ldb_msg_remove_attr(msg, "isDeleted");
        ldb_msg_remove_attr(msg, "isRecycled");
 
+       /* get the msg->element[] for the link attribute being processed */
        old_el = ldb_msg_find_element(msg, attr->lDAPDisplayName);
        if (old_el == NULL) {
                ret = ldb_msg_add_empty(msg, attr->lDAPDisplayName,
@@ -8260,12 +8279,18 @@ static int replmd_process_la_group(struct ldb_module *module,
                old_el->flags = LDB_FLAG_MOD_REPLACE;
        }
 
-       /* go through and process the link targets for this source object */
+       /*
+        * go through and process the link target value(s) for this particular
+        * source object and attribute
+        */
        for (la = DLIST_TAIL(la_group->la_entries); la; la=prev) {
                prev = DLIST_PREV(la);
                DLIST_REMOVE(la_group->la_entries, la);
 
-               /* parse the existing links */
+               /*
+                * parse the existing links (this can be costly for a large
+                * group, so we try to minimize the times we do it)
+                */
                if (pdn_list == NULL) {
                        ret = get_parsed_dns_trusted(module, replmd_private,
                                                     tmp_ctx, old_el,
@@ -8315,6 +8340,13 @@ static int replmd_process_la_group(struct ldb_module *module,
                return LDB_SUCCESS;
        }
 
+       /*
+        * Note that adding the whenChanged/etc attributes below will realloc
+        * msg->elements, invalidating the existing element/parsed-DN pointers
+        */
+       old_el = NULL;
+       TALLOC_FREE(pdn_list);
+
        /* update whenChanged/uSNChanged as the object has changed */
        t = time(NULL);
        ret = ldb_sequence_number(ldb, LDB_SEQ_HIGHEST_SEQ,