[SCM] Samba Shared Repository - branch master updated

Andrew Bartlett abartlet at samba.org
Fri Nov 23 07:11:02 UTC 2018


The branch, master has been updated
       via  6b136a8184d replmd: remove unnecessary indent
       via  9d5af87e66c replmd: Move logic into new replmd_should_apply_isDeleted() function
       via  b9c5417b523 replmd: Avoid passing msg to replmd_process_linked_attribute()
       via  1a45e5b136b tests: Fix flappiness in DRS tests due to RID Set changing
       via  b8ff4a0881b tests: Add better error for DRS tests
      from  6f48bc840c1 librpc:ndr: Fix undefined behavior in ndr.c

https://git.samba.org/?p=samba.git;a=shortlog;h=master


- Log -----------------------------------------------------------------
commit 6b136a8184d6f703405d0f53613b061af3601725
Author: Tim Beale <timbeale at catalyst.net.nz>
Date:   Tue Nov 20 16:02:05 2018 +1300

    replmd: remove unnecessary indent
    
    The previous refactor now means we return early if we don't need to
    re-apply isDeleted to the object. The 'else' is redundant and we can
    remove it to avoid unnecessary indent.
    
    This patch is basically just a whitespace change. It should not alter
    functionality.
    
    Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
    Reviewed-by: Andrew Bartlett <abartlet at samba.org>
    
    Autobuild-User(master): Andrew Bartlett <abartlet at samba.org>
    Autobuild-Date(master): Fri Nov 23 08:10:41 CET 2018 on sn-devel-144

commit 9d5af87e66c4e936ddd06607cc98f20c24e035b1
Author: Tim Beale <timbeale at catalyst.net.nz>
Date:   Tue Nov 20 15:54:31 2018 +1300

    replmd: Move logic into new replmd_should_apply_isDeleted() function
    
    It's easier to follow the logic involved here when it's split out into a
    separate function.
    
    This patch should not alter the existing logic/functionality.
    
    Note the 'else' case is somewhat redundant, but it avoids excessive
    whitespace changes to the function. It'll be tidied up in the next
    patch.
    
    Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
    Reviewed-by: Andrew Bartlett <abartlet at samba.org>

commit b9c5417b523c4c53cb275c12ec84bbc849705bec
Author: Tim Beale <timbeale at catalyst.net.nz>
Date:   Tue Nov 20 11:45:07 2018 +1300

    replmd: Avoid passing msg to replmd_process_linked_attribute()
    
    We can prevent anyone from inadvertently adding/removing msg->elements[]
    in replmd_process_linked_attribute() by just not passing msg into the
    function. Currently we only actually need the source DN and a memory
    context for reallocating old_el->values.
    
    The warning comment has been moved to a more appropriate place.
    
    Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
    Reviewed-by: Andrew Bartlett <abartlet at samba.org>

commit 1a45e5b136bedfed1fa3a8b87b79da759c3958c4
Author: Tim Beale <timbeale at catalyst.net.nz>
Date:   Tue Nov 20 17:30:37 2018 +1300

    tests: Fix flappiness in DRS tests due to RID Set changing
    
    The test_link_utdv_hwm test case in getnc_exop has started getting
    slightly flappy (8 failures in the last 2 weeks). The problem is the
    test case creates a new computer, which can occasionally result in a new
    RID pool being allocated.
    
    The problem can be reproduced by running the test case repeatedly (it
    usually fails after ~250 times).
    
    This patch updates the _check_ctr6() assertion to filter out the 'CN=RID
    Set' object, if it happens to be present.
    
    Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
    Reviewed-by: Andrew Bartlett <abartlet at samba.org>

commit b8ff4a0881bd6a12ba43ece27b8d3d9e5750b81f
Author: Tim Beale <timbeale at catalyst.net.nz>
Date:   Tue Nov 20 17:15:41 2018 +1300

    tests: Add better error for DRS tests
    
    We've got a flappy test hitting this assertion failure, but we can't
    tell why it's failing intermittently (probably because we're bumping the
    RID-Set, but there's no way to confirm this).
    
    Add some extra debug info if the test assertion fails.
    
    Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
    Reviewed-by: Andrew Bartlett <abartlet at samba.org>

-----------------------------------------------------------------------

Summary of changes:
 source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 195 ++++++++++++++----------
 source4/torture/drs/python/drs_base.py          |  17 ++-
 2 files changed, 132 insertions(+), 80 deletions(-)


Changeset truncated at 500 lines:

diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index d89776e4bc6..cfa63af7066 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -6798,6 +6798,45 @@ static int replmd_replicated_apply_next(struct replmd_replicated_request *ar)
 	return ldb_next_request(ar->module, search_req);
 }
 
+/*
+ * Returns true if we need to do extra processing to handle deleted object
+ * changes received via replication
+ */
+static bool replmd_should_apply_isDeleted(struct replmd_replicated_request *ar,
+					  struct ldb_message *msg)
+{
+	struct ldb_dn *deleted_objects_dn;
+	int ret;
+
+	if (!ar->isDeleted) {
+
+		/* not a deleted object, so don't set isDeleted */
+		return false;
+	}
+
+	ret = dsdb_get_deleted_objects_dn(ldb_module_get_ctx(ar->module),
+					  msg, msg->dn,
+					  &deleted_objects_dn);
+
+	/*
+	 * if the Deleted Object container lookup failed, then just apply
+	 * isDeleted (note that it doesn't exist for the Schema partition)
+	 */
+	if (ret != LDB_SUCCESS) {
+		return true;
+	}
+
+	/*
+	 * the Deleted Objects container has isDeleted set but is not entirely
+	 * a deleted object, so DON'T re-apply isDeleted to it
+	 */
+	if (ldb_dn_compare(msg->dn, deleted_objects_dn) == 0) {
+		return false;
+	}
+
+	return true;
+}
+
 /*
  * This is essentially a wrapper for replmd_replicated_apply_next()
  *
@@ -6805,79 +6844,77 @@ static int replmd_replicated_apply_next(struct replmd_replicated_request *ar)
  */
 static int replmd_replicated_apply_isDeleted(struct replmd_replicated_request *ar)
 {
-	struct ldb_dn *deleted_objects_dn;
 	struct ldb_message *msg = ar->objs->objects[ar->index_current].msg;
 	int ret;
+	bool apply_isDeleted;
+	struct ldb_request *del_req = NULL;
+	struct ldb_result *res = NULL;
+	TALLOC_CTX *tmp_ctx = NULL;
 
-	if (!ar->isDeleted) {
+	apply_isDeleted = replmd_should_apply_isDeleted(ar, msg);
 
-		/* not a deleted object, so nothing to do */
+	if (!apply_isDeleted) {
+
+		/* nothing to do */
 		ar->index_current++;
 		return replmd_replicated_apply_next(ar);
 	}
 
-	ret = dsdb_get_deleted_objects_dn(ldb_module_get_ctx(ar->module), msg,
-					  msg->dn, &deleted_objects_dn);
-	if (ret != LDB_SUCCESS || ldb_dn_compare(msg->dn, deleted_objects_dn) != 0) {
-		/*
-		 * Do a delete here again, so that if there is
-		 * anything local that conflicts with this
-		 * object being deleted, it is removed.  This
-		 * includes links.  See MS-DRSR 4.1.10.6.9
-		 * UpdateObject.
-		 *
-		 * If the object is already deleted, and there
-		 * is no more work required, it doesn't do
-		 * anything.
-		 */
-
-		/* This has been updated to point to the DN we eventually did the modify on */
+	/*
+	 * Do a delete here again, so that if there is
+	 * anything local that conflicts with this
+	 * object being deleted, it is removed.  This
+	 * includes links.  See MS-DRSR 4.1.10.6.9
+	 * UpdateObject.
+	 *
+	 * If the object is already deleted, and there
+	 * is no more work required, it doesn't do
+	 * anything.
+	 */
 
-		struct ldb_request *del_req;
-		struct ldb_result *res;
+	/* This has been updated to point to the DN we eventually did the modify on */
 
-		TALLOC_CTX *tmp_ctx = talloc_new(ar);
-		if (!tmp_ctx) {
-			ret = ldb_oom(ldb_module_get_ctx(ar->module));
-			return ret;
-		}
+	tmp_ctx = talloc_new(ar);
+	if (!tmp_ctx) {
+		ret = ldb_oom(ldb_module_get_ctx(ar->module));
+		return ret;
+	}
 
-		res = talloc_zero(tmp_ctx, struct ldb_result);
-		if (!res) {
-			ret = ldb_oom(ldb_module_get_ctx(ar->module));
-			talloc_free(tmp_ctx);
-			return ret;
-		}
+	res = talloc_zero(tmp_ctx, struct ldb_result);
+	if (!res) {
+		ret = ldb_oom(ldb_module_get_ctx(ar->module));
+		talloc_free(tmp_ctx);
+		return ret;
+	}
 
-		/* Build a delete request, which hopefully will artually turn into nothing */
-		ret = ldb_build_del_req(&del_req, ldb_module_get_ctx(ar->module), tmp_ctx,
-					msg->dn,
-					NULL,
-					res,
-					ldb_modify_default_callback,
-					ar->req);
-		LDB_REQ_SET_LOCATION(del_req);
-		if (ret != LDB_SUCCESS) {
-			talloc_free(tmp_ctx);
-			return ret;
-		}
+	/* Build a delete request, which hopefully will artually turn into nothing */
+	ret = ldb_build_del_req(&del_req, ldb_module_get_ctx(ar->module), tmp_ctx,
+				msg->dn,
+				NULL,
+				res,
+				ldb_modify_default_callback,
+				ar->req);
+	LDB_REQ_SET_LOCATION(del_req);
+	if (ret != LDB_SUCCESS) {
+		talloc_free(tmp_ctx);
+		return ret;
+	}
 
-		/*
-		 * This is the guts of the call, call back
-		 * into our delete code, but setting the
-		 * re_delete flag so we delete anything that
-		 * shouldn't be there on a deleted or recycled
-		 * object
-		 */
-		ret = replmd_delete_internals(ar->module, del_req, true);
-		if (ret == LDB_SUCCESS) {
-			ret = ldb_wait(del_req->handle, LDB_WAIT_ALL);
-		}
+	/*
+	 * This is the guts of the call, call back
+	 * into our delete code, but setting the
+	 * re_delete flag so we delete anything that
+	 * shouldn't be there on a deleted or recycled
+	 * object
+	 */
+	ret = replmd_delete_internals(ar->module, del_req, true);
+	if (ret == LDB_SUCCESS) {
+		ret = ldb_wait(del_req->handle, LDB_WAIT_ALL);
+	}
 
-		talloc_free(tmp_ctx);
-		if (ret != LDB_SUCCESS) {
-			return ret;
-		}
+	talloc_free(tmp_ctx);
+	if (ret != LDB_SUCCESS) {
+		return ret;
 	}
 
 	ar->index_current++;
@@ -7939,13 +7976,11 @@ static int replmd_check_singleval_la_conflict(struct ldb_module *module,
 
 /**
  * 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 src_dn the DN of the source object for the link
  * @param attr schema info for the linked attribute
  * @param la_entry the linked attribute info received via DRS
+ * @param element_ctx mem context for msg->element[] (when adding a new value
+ * we need to realloc old_el->values)
  * @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.
@@ -7956,11 +7991,12 @@ static int replmd_check_singleval_la_conflict(struct ldb_module *module,
 static int replmd_process_linked_attribute(struct ldb_module *module,
 					   TALLOC_CTX *mem_ctx,
 					   struct replmd_private *replmd_private,
-					   struct ldb_message *msg,
+					   struct ldb_dn *src_dn,
 					   const struct dsdb_attribute *attr,
 					   struct la_entry *la_entry,
 					   struct ldb_request *parent,
 					   struct ldb_message_element *old_el,
+					   TALLOC_CTX *element_ctx,
 					   struct parsed_dn *pdn_list,
 					   replmd_link_changed *change)
 {
@@ -7987,12 +8023,12 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
 	if (!W_ERROR_IS_OK(status)) {
 		ldb_asprintf_errstring(ldb, "Failed to parsed linked attribute blob for %s on %s - %s\n",
 				       attr->lDAPDisplayName,
-				       ldb_dn_get_linearized(msg->dn),
+				       ldb_dn_get_linearized(src_dn),
 				       win_errstr(status));
 		return LDB_ERR_OPERATIONS_ERROR;
 	}
 
-	ret = replmd_check_target_exists(module, dsdb_dn, la_entry, msg->dn,
+	ret = replmd_check_target_exists(module, dsdb_dn, la_entry, src_dn,
 					 true, &guid, &ignore_link);
 
 	if (ret != LDB_SUCCESS) {
@@ -8021,7 +8057,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
 
 	if (!replmd_link_update_is_newer(pdn, la)) {
 		DEBUG(3,("Discarding older DRS linked attribute update to %s on %s from %s\n",
-			 old_el->name, ldb_dn_get_linearized(msg->dn),
+			 old_el->name, ldb_dn_get_linearized(src_dn),
 			 GUID_string(mem_ctx, &la->meta_data.originating_invocation_id)));
 		return LDB_SUCCESS;
 	}
@@ -8038,7 +8074,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
 	 */
 	if (active) {
 		ret = replmd_check_singleval_la_conflict(module, replmd_private,
-							 mem_ctx, msg->dn, la,
+							 mem_ctx, src_dn, la,
 							 dsdb_dn, pdn, pdn_list,
 							 old_el, schema, attr,
 							 seq_num,
@@ -8055,7 +8091,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
 			/* remove the existing backlink */
 			ret = replmd_add_backlink(module, replmd_private,
 						  schema, 
-						  msg->dn,
+						  src_dn,
 						  &pdn->guid, false, attr,
 						  parent);
 			if (ret != LDB_SUCCESS) {
@@ -8090,7 +8126,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
 			}
 		}
 
-		old_el->values = talloc_realloc(msg->elements, old_el->values,
+		old_el->values = talloc_realloc(element_ctx, old_el->values,
 						struct ldb_val, old_el->num_values+1);
 		if (!old_el->values) {
 			ldb_module_oom(module);
@@ -8124,7 +8160,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
 
 		/* Set the new link as inactive/deleted to avoid conflicts */
 		ret = replmd_delete_link_value(module, replmd_private, old_el,
-					       msg->dn, schema, attr, seq_num,
+					       src_dn, schema, attr, seq_num,
 					       false, &guid, dsdb_dn,
 					       val_to_update);
 
@@ -8137,7 +8173,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
 		/* if the new link is active, then add the new backlink */
 		ret = replmd_add_backlink(module, replmd_private,
 					  schema,
-					  msg->dn,
+					  src_dn,
 					  &guid, true, attr,
 					  parent);
 		if (ret != LDB_SUCCESS) {
@@ -8281,7 +8317,12 @@ static int replmd_process_la_group(struct ldb_module *module,
 
 	/*
 	 * go through and process the link target value(s) for this particular
-	 * source object and attribute
+	 * source object and attribute. For optimization, the same msg is used
+	 * across multiple calls to replmd_process_linked_attribute().
+	 * Note that we should not add or remove any msg attributes inside the
+	 * loop (we should only add/modify *values* for the attribute being
+	 * processed). Otherwise msg->elements is realloc'd and old_el/pdn_list
+	 * pointers will be invalidated
 	 */
 	for (la = DLIST_TAIL(la_group->la_entries); la; la=prev) {
 		prev = DLIST_PREV(la);
@@ -8304,9 +8345,9 @@ static int replmd_process_la_group(struct ldb_module *module,
 		}
 		ret = replmd_process_linked_attribute(module, tmp_ctx,
 						      replmd_private,
-						      msg, attr, la, NULL,
-						      old_el, pdn_list,
-						      &change_type);
+						      msg->dn, attr, la, NULL,
+						      msg->elements, old_el,
+						      pdn_list, &change_type);
 		if (ret != LDB_SUCCESS) {
 			replmd_txn_cleanup(replmd_private);
 			return ret;
diff --git a/source4/torture/drs/python/drs_base.py b/source4/torture/drs/python/drs_base.py
index 2e8598d529b..d19e625021b 100644
--- a/source4/torture/drs/python/drs_base.py
+++ b/source4/torture/drs/python/drs_base.py
@@ -384,15 +384,26 @@ class DrsBaseTestCase(SambaToolCmdTest):
         """
         Check that a ctr6 matches the specified parameters.
         """
-        self.assertEqual(ctr6.object_count, len(expected_dns))
+        ctr6_raw_dns = self._get_ctr6_dn_list(ctr6)
+
+        # filter out changes to the RID Set objects, as these can happen
+        # intermittently and mess up the test assertions
+        ctr6_dns = []
+        for dn in ctr6_raw_dns:
+            if "CN=RID Set," in dn or "CN=RID Manager$," in dn:
+                print("Removing {0} from GetNCChanges reply".format(dn))
+            else:
+                ctr6_dns.append(dn)
+
+        self.assertEqual(len(ctr6_dns), len(expected_dns),
+                         "Received unexpected objects (%s)" % ctr6_dns)
+        self.assertEqual(ctr6.object_count, len(ctr6_raw_dns))
         self.assertEqual(ctr6.linked_attributes_count, len(expected_links))
         self.assertEqual(ctr6.more_data, more_data)
         self.assertEqual(ctr6.nc_object_count, nc_object_count)
         self.assertEqual(ctr6.nc_linked_attributes_count, nc_linked_attributes_count)
         self.assertEqual(ctr6.drs_error[0], drs_error)
 
-        ctr6_dns = self._get_ctr6_dn_list(ctr6)
-
         i = 0
         for dn in expected_dns:
             # Expect them back in the exact same order as specified.


-- 
Samba Shared Repository



More information about the samba-cvs mailing list