From: Tim Beale Date: Mon, 19 Nov 2018 21:59:40 +0000 (+1300) Subject: replmd: Make replmd_process_linked_attribute() mem dependencies clearer X-Git-Tag: tdb-1.3.17~703 X-Git-Url: http://git.samba.org/?a=commitdiff_plain;h=a370f217bb94601345ad5700ea546259e1d04bfd;p=samba.git 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 Reviewed-by: Andrew Bartlett Autobuild-User(master): Tim Beale 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 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,