Attachment 13801 Details for Bug 13156 – Patch to fix the race condition (in the tests)

[patch] Patch to fix the race condition (in the tests)

patch (text/plain), 16.75 KB, created by Garming Sam on 2017-11-22 00:51:34 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Garming Sam

Created: 2017-11-22 00:51:34 UTC

Size: 16.75 KB

patch

obsolete

>From 5a1d1cdade30adf001ff2d18e298edabfe5e75b5 Mon Sep 17 00:00:00 2001
>From: Garming Sam <garming@catalyst.net.nz>
>Date: Wed, 22 Nov 2017 12:34:01 +1300
>Subject: [PATCH 1/2] schema: Make writing indices flag an enum for a new state
>
>In schema_load_init, we find that the writing of indices is not locked
>in any way. This leads to race conditions. To resolve this, we need to
>have a new state (SCHEMA_COMPARE) which can report to the caller that we
>need to open a transaction to write the indices.
>
>Signed-off-by: Garming Sam <garming@catalyst.net.nz>
>---
> source4/dsdb/pydsdb.c                        |  2 +-
> source4/dsdb/repl/replicated_objects.c       | 14 +++----
> source4/dsdb/samdb/ldb_modules/schema_load.c | 63 ++++++++++++++++++++++++----
> source4/dsdb/schema/schema.h                 |  6 +++
> source4/dsdb/schema/schema_set.c             | 41 +++++++++++++++---
> source4/dsdb/schema/tests/schema_syntax.c    |  2 +-
> source4/libnet/libnet_vampire.c              |  4 +-
> source4/torture/drs/drs_util.c               |  2 +-
> 8 files changed, 108 insertions(+), 26 deletions(-)
>
>diff --git a/source4/dsdb/pydsdb.c b/source4/dsdb/pydsdb.c
>index 09623a6..4d811b0 100644
>--- a/source4/dsdb/pydsdb.c
>+++ b/source4/dsdb/pydsdb.c
>@@ -919,7 +919,7 @@ static PyObject *py_dsdb_set_schema_from_ldb(PyObject *self, PyObject *args)
> 	struct ldb_context *from_ldb;
> 	struct dsdb_schema *schema;
> 	int ret;
>-	char write_indices_and_attributes = true;
>+	char write_indices_and_attributes = SCHEMA_WRITE;
> 	if (!PyArg_ParseTuple(args, "OO|b",
> 			      &py_ldb, &py_from_ldb, &write_indices_and_attributes))
> 		return NULL;
>diff --git a/source4/dsdb/repl/replicated_objects.c b/source4/dsdb/repl/replicated_objects.c
>index d9365ae..0c44960 100644
>--- a/source4/dsdb/repl/replicated_objects.c
>+++ b/source4/dsdb/repl/replicated_objects.c
>@@ -867,7 +867,7 @@ WERROR dsdb_replicated_objects_commit(struct ldb_context *ldb,
> 		cur_schema = dsdb_get_schema(ldb, tmp_ctx);
> 		used_global_schema = dsdb_uses_global_schema(ldb);
> 
>-		ret = dsdb_reference_schema(ldb, working_schema, false);
>+		ret = dsdb_reference_schema(ldb, working_schema, SCHEMA_MEMORY_ONLY);
> 		if (ret != LDB_SUCCESS) {
> 			DEBUG(0,(__location__ "Failed to reference working schema - %s\n",
> 				 ldb_strerror(ret)));
>@@ -884,7 +884,7 @@ WERROR dsdb_replicated_objects_commit(struct ldb_context *ldb,
> 		if (used_global_schema) { 
> 			dsdb_set_global_schema(ldb);
> 		} else if (cur_schema) {
>-			dsdb_reference_schema(ldb, cur_schema, false);
>+			dsdb_reference_schema(ldb, cur_schema, SCHEMA_MEMORY_ONLY);
> 		}
> 
> 		if (W_ERROR_EQUAL(objects->error, WERR_DS_DRA_RECYCLED_TARGET)) {
>@@ -917,7 +917,7 @@ WERROR dsdb_replicated_objects_commit(struct ldb_context *ldb,
> 			if (used_global_schema) { 
> 				dsdb_set_global_schema(ldb);
> 			} else if (cur_schema ) {
>-				dsdb_reference_schema(ldb, cur_schema, false);
>+				dsdb_reference_schema(ldb, cur_schema, SCHEMA_MEMORY_ONLY);
> 			}
> 			DEBUG(0,("Failed to save updated prefixMap: %s\n",
> 				 win_errstr(werr)));
>@@ -932,7 +932,7 @@ WERROR dsdb_replicated_objects_commit(struct ldb_context *ldb,
> 		if (used_global_schema) { 
> 			dsdb_set_global_schema(ldb);
> 		} else if (cur_schema ) {
>-			dsdb_reference_schema(ldb, cur_schema, false);
>+			dsdb_reference_schema(ldb, cur_schema, SCHEMA_MEMORY_ONLY);
> 		}
> 		DEBUG(0,(__location__ " Failed to prepare commit of transaction: %s\n",
> 			 ldb_errstring(ldb)));
>@@ -946,7 +946,7 @@ WERROR dsdb_replicated_objects_commit(struct ldb_context *ldb,
> 		if (used_global_schema) { 
> 			dsdb_set_global_schema(ldb);
> 		} else if (cur_schema ) {
>-			dsdb_reference_schema(ldb, cur_schema, false);
>+			dsdb_reference_schema(ldb, cur_schema, SCHEMA_MEMORY_ONLY);
> 		}
> 		DEBUG(0,(__location__ " Failed to load partition uSN\n"));
> 		ldb_transaction_cancel(ldb);
>@@ -960,7 +960,7 @@ WERROR dsdb_replicated_objects_commit(struct ldb_context *ldb,
> 		if (used_global_schema) { 
> 			dsdb_set_global_schema(ldb);
> 		} else if (cur_schema ) {
>-			dsdb_reference_schema(ldb, cur_schema, false);
>+			dsdb_reference_schema(ldb, cur_schema, SCHEMA_MEMORY_ONLY);
> 		}
> 		DEBUG(0,(__location__ " Failed to commit transaction\n"));
> 		TALLOC_FREE(tmp_ctx);
>@@ -1005,7 +1005,7 @@ WERROR dsdb_replicated_objects_commit(struct ldb_context *ldb,
> 				new_schema != NULL ?
> 				new_schema->metadata_usn : 0,
> 				working_schema, working_schema->metadata_usn);
>-			dsdb_reference_schema(ldb, cur_schema, false);
>+			dsdb_reference_schema(ldb, cur_schema, SCHEMA_MEMORY_ONLY);
> 			if (used_global_schema) {
> 				dsdb_set_global_schema(ldb);
> 			}
>diff --git a/source4/dsdb/samdb/ldb_modules/schema_load.c b/source4/dsdb/samdb/ldb_modules/schema_load.c
>index b3313b4..4013cdf 100644
>--- a/source4/dsdb/samdb/ldb_modules/schema_load.c
>+++ b/source4/dsdb/samdb/ldb_modules/schema_load.c
>@@ -246,7 +246,7 @@ static struct dsdb_schema *dsdb_schema_refresh(struct ldb_module *module, struct
> 		return schema;
> 	}
> 
>-	ret = dsdb_set_schema(ldb, new_schema, false);
>+	ret = dsdb_set_schema(ldb, new_schema, SCHEMA_MEMORY_ONLY);
> 	if (ret != LDB_SUCCESS) {
> 		ldb_debug_set(ldb, LDB_DEBUG_FATAL,
> 			      "dsdb_set_schema() failed: %d:%s: %s",
>@@ -359,7 +359,8 @@ failed:
> }	
> 
> static int schema_load(struct ldb_context *ldb,
>-		       struct ldb_module *module)
>+		       struct ldb_module *module,
>+		       bool *need_write)
> {
> 	struct dsdb_schema *schema;
> 	void *readOnlySchema;
>@@ -406,7 +407,7 @@ static int schema_load(struct ldb_context *ldb,
> 		}
> 
> 		/* "dsdb_set_schema()" steals schema into the ldb_context */
>-		ret = dsdb_set_schema(ldb, new_schema, false);
>+		ret = dsdb_set_schema(ldb, new_schema, SCHEMA_MEMORY_ONLY);
> 		if (ret != LDB_SUCCESS) {
> 			ldb_debug_set(ldb, LDB_DEBUG_FATAL,
> 				      "schema_load_init: dsdb_set_schema() failed: %d:%s: %s",
>@@ -444,7 +445,14 @@ static int schema_load(struct ldb_context *ldb,
> 
> 	/* Now check the @INDEXLIST is correct, or fix it up */
> 	ret = dsdb_schema_set_indices_and_attributes(ldb, schema,
>-						     true);
>+						     SCHEMA_COMPARE);
>+	if (ret == LDB_ERR_BUSY) {
>+		*need_write = true;
>+		ret = LDB_SUCCESS;
>+	} else {
>+		*need_write = false;
>+	}
>+
> 	if (ret != LDB_SUCCESS) {
> 		ldb_asprintf_errstring(ldb, "Failed to update "
> 				       "@INDEXLIST and @ATTRIBUTES "
>@@ -463,6 +471,7 @@ static int schema_load_init(struct ldb_module *module)
> 	struct ldb_context *ldb = ldb_module_get_ctx(module);
> 	struct schema_load_private_data *private_data;
> 	int ret;
>+	bool need_write = false;
> 
> 	private_data = talloc_zero(module, struct schema_load_private_data);
> 	if (private_data == NULL) {
>@@ -477,11 +486,49 @@ static int schema_load_init(struct ldb_module *module)
> 		return ret;
> 	}
> 
>-	ret = schema_load(ldb, module);
>-	if (ret != LDB_SUCCESS) {
>-		return ret;
>+	ret = schema_load(ldb, module, &need_write);
>+
>+	if (ret == LDB_SUCCESS && need_write) {
>+		TALLOC_CTX *frame = talloc_stackframe();
>+		struct dsdb_schema *schema = NULL;
>+
>+		ret = ldb_transaction_start(ldb);
>+		if (ret != LDB_SUCCESS) {
>+			ldb_debug_set(ldb, LDB_DEBUG_FATAL,
>+				      "schema_load_init: transaction start failed");
>+			return LDB_ERR_OPERATIONS_ERROR;
>+		}
>+
>+		schema = dsdb_get_schema(ldb, frame);
>+		if (schema == NULL) {
>+			ldb_debug_set(ldb, LDB_DEBUG_FATAL,
>+				      "schema_load_init: dsdb_get_schema failed");
>+			ldb_transaction_cancel(ldb);
>+			return LDB_ERR_OPERATIONS_ERROR;
>+		}
>+		ret = dsdb_schema_set_indices_and_attributes(ldb, schema,
>+							     SCHEMA_WRITE);
>+
>+		TALLOC_FREE(frame);
>+
>+		if (ret != LDB_SUCCESS) {
>+			ldb_asprintf_errstring(ldb, "Failed to write new "
>+					       "@INDEXLIST and @ATTRIBUTES "
>+					       "records for updated schema: %s",
>+					       ldb_errstring(ldb));
>+			ldb_transaction_cancel(ldb);
>+			return ret;
>+		}
>+
>+		ret = ldb_transaction_commit(ldb);
>+		if (ret != LDB_SUCCESS) {
>+			ldb_debug_set(ldb, LDB_DEBUG_FATAL,
>+				      "schema_load_init: transaction commit failed");
>+			return LDB_ERR_OPERATIONS_ERROR;
>+		}
> 	}
> 
>+
> 	return ret;
> }
> 
>@@ -549,7 +596,7 @@ static int schema_load_extended(struct ldb_module *module, struct ldb_request *r
> 
> 	ret = dsdb_schema_set_indices_and_attributes(ldb,
> 						     schema,
>-						     true);
>+						     SCHEMA_WRITE);
> 
> 	if (ret != LDB_SUCCESS) {
> 		ldb_asprintf_errstring(ldb, "Failed to write new "
>diff --git a/source4/dsdb/schema/schema.h b/source4/dsdb/schema/schema.h
>index 75e4886..9b27fd2 100644
>--- a/source4/dsdb/schema/schema.h
>+++ b/source4/dsdb/schema/schema.h
>@@ -192,6 +192,12 @@ struct dsdb_class {
> 	} tmp;
> };
> 
>+enum schema_set_enum {
>+	SCHEMA_MEMORY_ONLY = 0,
>+	SCHEMA_WRITE = 1,
>+	SCHEMA_COMPARE = 2,
>+};
>+
> /**
>  * data stored in schemaInfo attribute
>  */
>diff --git a/source4/dsdb/schema/schema_set.c b/source4/dsdb/schema/schema_set.c
>index ca7a307..32cdd71 100644
>--- a/source4/dsdb/schema/schema_set.c
>+++ b/source4/dsdb/schema/schema_set.c
>@@ -58,7 +58,7 @@ const struct ldb_schema_attribute *dsdb_attribute_handler_override(struct ldb_co
>  */
> int dsdb_schema_set_indices_and_attributes(struct ldb_context *ldb,
> 					   struct dsdb_schema *schema,
>-					   bool write_indices_and_attributes)
>+					   enum schema_set_enum mode)
> {
> 	int ret = LDB_SUCCESS;
> 	struct ldb_result *res;
>@@ -80,7 +80,7 @@ int dsdb_schema_set_indices_and_attributes(struct ldb_context *ldb,
> 		ldb_schema_set_override_GUID_index(ldb, "objectGUID", "GUID");
> 	}
> 
>-	if (!write_indices_and_attributes) {
>+	if (mode == SCHEMA_MEMORY_ONLY) {
> 		return ret;
> 	}
> 
>@@ -178,9 +178,17 @@ int dsdb_schema_set_indices_and_attributes(struct ldb_context *ldb,
> 	ret = ldb_search(ldb, mem_ctx, &res, msg->dn, LDB_SCOPE_BASE, NULL,
> 			 NULL);
> 	if (ret == LDB_ERR_NO_SUCH_OBJECT) {
>+		if (mode == SCHEMA_COMPARE) {
>+			/* We are probably not in a transaction */
>+			goto wrong_mode;
>+		}
> 		ret = ldb_add(ldb, msg);
> 	} else if (ret != LDB_SUCCESS) {
> 	} else if (res->count != 1) {
>+		if (mode == SCHEMA_COMPARE) {
>+			/* We are probably not in a transaction */
>+			goto wrong_mode;
>+		}
> 		ret = ldb_add(ldb, msg);
> 	} else {
> 		ret = LDB_SUCCESS;
>@@ -198,6 +206,10 @@ int dsdb_schema_set_indices_and_attributes(struct ldb_context *ldb,
> 			 * are under the read lock and we wish to do a
> 			 * delete of any removed/renamed attributes
> 			 */
>+			if (mode == SCHEMA_COMPARE) {
>+				/* We are probably not in a transaction */
>+				goto wrong_mode;
>+			}
> 			ret = dsdb_modify(ldb, mod_msg, 0);
> 		}
> 		talloc_free(mod_msg);
>@@ -219,9 +231,17 @@ int dsdb_schema_set_indices_and_attributes(struct ldb_context *ldb,
> 	ret = ldb_search(ldb, mem_ctx, &res_idx, msg_idx->dn, LDB_SCOPE_BASE,
> 			 NULL, NULL);
> 	if (ret == LDB_ERR_NO_SUCH_OBJECT) {
>+		if (mode == SCHEMA_COMPARE) {
>+			/* We are probably not in a transaction */
>+			goto wrong_mode;
>+		}
> 		ret = ldb_add(ldb, msg_idx);
> 	} else if (ret != LDB_SUCCESS) {
> 	} else if (res_idx->count != 1) {
>+		if (mode == SCHEMA_COMPARE) {
>+			/* We are probably not in a transaction */
>+			goto wrong_mode;
>+		}
> 		ret = ldb_add(ldb, msg_idx);
> 	} else {
> 		ret = LDB_SUCCESS;
>@@ -260,6 +280,10 @@ int dsdb_schema_set_indices_and_attributes(struct ldb_context *ldb,
> 			 * are under the read lock and we wish to do a
> 			 * delete of any removed/renamed attributes
> 			 */
>+			if (mode == SCHEMA_COMPARE) {
>+				/* We are probably not in a transaction */
>+				goto wrong_mode;
>+			}
> 			ret = dsdb_modify(ldb, mod_msg, 0);
> 		}
> 		talloc_free(mod_msg);
>@@ -280,6 +304,10 @@ int dsdb_schema_set_indices_and_attributes(struct ldb_context *ldb,
> op_error:
> 	talloc_free(mem_ctx);
> 	return ldb_operr(ldb);
>+
>+wrong_mode:
>+	talloc_free(mem_ctx);
>+	return LDB_ERR_BUSY;
> }
> 
> 
>@@ -531,7 +559,7 @@ int dsdb_set_schema_refresh_function(struct ldb_context *ldb,
>  */
> int dsdb_set_schema(struct ldb_context *ldb,
> 		    struct dsdb_schema *schema,
>-		    bool write_indices_and_attributes)
>+		    enum schema_set_enum write_indices_and_attributes)
> {
> 	struct dsdb_schema *old_schema;
> 	int ret;
>@@ -586,7 +614,7 @@ static struct dsdb_schema *global_schema;
>  * disk.
>  */
> int dsdb_reference_schema(struct ldb_context *ldb, struct dsdb_schema *schema,
>-			  bool write_indices_and_attributes)
>+			  enum schema_set_enum write_indices_and_attributes)
> {
> 	int ret;
> 	struct dsdb_schema *old_schema;
>@@ -657,7 +685,8 @@ int dsdb_set_global_schema(struct ldb_context *ldb)
> 	talloc_unlink(ldb, old_schema);
> 
> 	/* Set the new attributes based on the new schema */
>-	ret = dsdb_schema_set_indices_and_attributes(ldb, global_schema, false /* Don't write indices and attributes, it's expensive */);
>+	/* Don't write indices and attributes, it's expensive */
>+	ret = dsdb_schema_set_indices_and_attributes(ldb, global_schema, SCHEMA_MEMORY_ONLY);
> 	if (ret == LDB_SUCCESS) {
> 		/* Keep a reference to this schema, just in case the original copy is replaced */
> 		if (talloc_reference(ldb, global_schema) == NULL) {
>@@ -948,7 +977,7 @@ WERROR dsdb_set_schema_from_ldif(struct ldb_context *ldb,
> 		}
> 	}
> 
>-	ret = dsdb_set_schema(ldb, schema, true);
>+	ret = dsdb_set_schema(ldb, schema, SCHEMA_WRITE);
> 	if (ret != LDB_SUCCESS) {
> 		status = WERR_FOOBAR;
> 		DEBUG(0,("ERROR: dsdb_set_schema() failed with %s / %s\n",
>diff --git a/source4/dsdb/schema/tests/schema_syntax.c b/source4/dsdb/schema/tests/schema_syntax.c
>index cb4d802..17e6201 100644
>--- a/source4/dsdb/schema/tests/schema_syntax.c
>+++ b/source4/dsdb/schema/tests/schema_syntax.c
>@@ -78,7 +78,7 @@ static bool torture_syntax_add_OR_Name(struct torture_context *tctx,
> 	ldb_ldif_read_free(ldb, ldif);
> 	torture_assert_werr_ok(tctx, werr, "dsdb_set_attribute_from_ldb() failed!");
> 
>-	ldb_res = dsdb_set_schema(ldb, schema, true);
>+	ldb_res = dsdb_set_schema(ldb, schema, SCHEMA_WRITE);
> 	torture_assert_int_equal(tctx, ldb_res, LDB_SUCCESS, "dsdb_set_schema() failed");
> 
> 	return true;
>diff --git a/source4/libnet/libnet_vampire.c b/source4/libnet/libnet_vampire.c
>index a449e1c..2871a68 100644
>--- a/source4/libnet/libnet_vampire.c
>+++ b/source4/libnet/libnet_vampire.c
>@@ -331,7 +331,7 @@ static WERROR libnet_vampire_cb_apply_schema(struct libnet_vampire_cb_state *s,
> 		provision_schema = dsdb_get_schema(s->ldb, s);
> 	} else {
> 		provision_schema = dsdb_get_schema(schema_ldb, s);
>-		ret = dsdb_reference_schema(s->ldb, provision_schema, false);
>+		ret = dsdb_reference_schema(s->ldb, provision_schema, SCHEMA_MEMORY_ONLY);
> 		if (ret != LDB_SUCCESS) {
> 			DEBUG(0,("Failed to attach schema from local provision using remote prefixMap."));
> 			return WERR_INTERNAL_ERROR;
>@@ -368,7 +368,7 @@ static WERROR libnet_vampire_cb_apply_schema(struct libnet_vampire_cb_state *s,
> 	 * attach the schema we just brought over DRS to the ldb,
> 	 * so we can use it in dsdb_convert_object_ex below
> 	 */
>-	ret = dsdb_set_schema(s->ldb, s->self_made_schema, true);
>+	ret = dsdb_set_schema(s->ldb, s->self_made_schema, SCHEMA_WRITE);
> 	if (ret != LDB_SUCCESS) {
> 		DEBUG(0,("Failed to attach working schema from DRS.\n"));
> 		return WERR_INTERNAL_ERROR;
>diff --git a/source4/torture/drs/drs_util.c b/source4/torture/drs/drs_util.c
>index 7c073c9..4795bd9 100644
>--- a/source4/torture/drs/drs_util.c
>+++ b/source4/torture/drs/drs_util.c
>@@ -158,7 +158,7 @@ bool drs_util_dsdb_schema_load_ldb(struct torture_context *tctx,
> 
> 	talloc_free(res);
> 
>-	ret = dsdb_set_schema(ldb, ldap_schema, true);
>+	ret = dsdb_set_schema(ldb, ldap_schema, SCHEMA_WRITE);
> 	if (ret != LDB_SUCCESS) {
> 		torture_fail(tctx,
> 			     talloc_asprintf(tctx, "dsdb_set_schema() failed: %s", ldb_strerror(ret)));
>-- 
>1.9.1
>
>
>From 9d2e2039bbf4c44b5134802fb2c5a17a3f64f9bd Mon Sep 17 00:00:00 2001
>From: Garming Sam <garming@catalyst.net.nz>
>Date: Wed, 22 Nov 2017 12:46:31 +1300
>Subject: [PATCH 2/2] schema_set: Add comment about set schema from ldif in a
> transaction
>
>This is normally called with a transaction or before access is shared.
>The python code and some tests may also cause an issue, but as these are
>fixed at runtime, this is only a temporary issue that resolves itself.
>
>Signed-off-by: Garming Sam <garming@catalyst.net.nz>
>---
> source4/dsdb/schema/schema_set.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/source4/dsdb/schema/schema_set.c b/source4/dsdb/schema/schema_set.c
>index 32cdd71..226e31e 100644
>--- a/source4/dsdb/schema/schema_set.c
>+++ b/source4/dsdb/schema/schema_set.c
>@@ -892,6 +892,9 @@ WERROR dsdb_schema_set_el_from_ldb_msg(struct ldb_context *ldb,
>  * Rather than read a schema from the LDB itself, read it from an ldif
>  * file.  This allows schema to be loaded and used while adding the
>  * schema itself to the directory.
>+ *
>+ * Should be called with a transaction (or failing that, have no concurrent
>+ * access while called).
>  */
> 
> WERROR dsdb_set_schema_from_ldif(struct ldb_context *ldb,
>@@ -977,6 +980,13 @@ WERROR dsdb_set_schema_from_ldif(struct ldb_context *ldb,
> 		}
> 	}
> 
>+	/*
>+	 * TODO We may need a transaction here, otherwise this causes races.
>+	 *
>+	 * To do so may require an ldb_in_transaction function. In the
>+	 * meantime, assume that this is always called with a transaction or in
>+	 * isolation.
>+	 */
> 	ret = dsdb_set_schema(ldb, schema, SCHEMA_WRITE);
> 	if (ret != LDB_SUCCESS) {
> 		status = WERR_FOOBAR;
>-- 
>1.9.1
>

Actions: View

Attachments on bug 13156: 13801