From 5a1d1cdade30adf001ff2d18e298edabfe5e75b5 Mon Sep 17 00:00:00 2001 From: Garming Sam 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 --- 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 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 --- 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