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

1 files changed, 38 insertions, 6 deletions

diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index b88aaf251e7..d89776e4bc6 100644
--- 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,