[SCM] Samba Shared Repository - branch master updated
Douglas Bagnall
dbagnall at samba.org
Sat Jul 27 23:52:02 UTC 2024
The branch, master has been updated
via e58e4a5aa99 ldb:kv_index: use subtransaction_cancel in transaction_cancel
via 18131aebf81 ldb:kv_index: subtransaction_cancel: check for nested tdb
via 9ecf96859fe ldb:kv_index: don't recalculate a length
via 1bf9ede94f0 ldb:kv_index: realloc away old dn list
via 5f0198d6984 ldb_kv_index: dn_list load sub transaction can re-use keys
via ed7bc50b00e ldb:ldb_kv_dn_list_find_val: check for int overflow
via d8c7768fed5 ldb_kv_cache: always initialise dn_list.strict
via 351636efa7f tdb: allow tracing of internal tdb
via e653b0870fd tdb: fix compilation with TDB_TRACE=1
from e61f53b656f WHATSNEW: Automatic keytab update after machine password changes
https://git.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit e58e4a5aa996e787fa1488347dc3121a15cb0321
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Sun Jul 21 18:04:49 2024 +1200
ldb:kv_index: use subtransaction_cancel in transaction_cancel
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
Autobuild-User(master): Douglas Bagnall <dbagnall at samba.org>
Autobuild-Date(master): Sat Jul 27 23:51:44 UTC 2024 on atb-devel-224
commit 18131aebf81ab7cce1faeaa13a56067fb666d1f5
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Sun Jul 21 18:07:56 2024 +1200
ldb:kv_index: subtransaction_cancel: check for nested tdb
Just in case, but also so ldb_kv_index_transaction_cancel() can use
this and retain the same logic.
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit 9ecf96859fe00520ac6baef4f4045f21d2ad7413
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Sun Jul 21 18:06:18 2024 +1200
ldb:kv_index: don't recalculate a length
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit 1bf9ede94f0a6b41fb18e880e59a8e390f8c21d3
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Mon Jul 22 22:22:15 2024 +1200
ldb:kv_index: realloc away old dn list
We can't just free it, because has the GUID index list as a child, and
these are shared by the new dn list (from the subtransaction we are
committing). But if the dn list is long and the main transaction is
long-lived, we can save a lot of memory by turning this dn list into
an almost empty node in the talloc tree. This returns us to roughly
the situation we had prior to the last commit.
For example, with the repro.sh script on bug 15590 in indexes mode
with 10000 rules, The last 3 commits use this much memory at the end
of an unusually large transaction:
full talloc report on 'struct ldb_context' (total 4012222 bytes in 90058 blocks)
full talloc report on 'struct ldb_context' (total 2405482219 bytes in 90058 blocks)
full talloc report on 'struct ldb_context' (total 4282195 bytes in 90058 blocks)
That is, the last commit increased usage 500 fold, and this commit
brings it back to normal.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15590
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit 5f0198d69843c864f2b98a7c0c6305ad789a68a0
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed Jun 26 11:05:49 2024 +1200
ldb_kv_index: dn_list load sub transaction can re-use keys
We don't want to modify the original list, but we can reuse the keys
if we treat them as immutable and don't free them. That makes it a lot
quicker if there are many keys (i.e. where an index is useful) and may
sub-transactions. In particular, it avoids O(n²) talloc_memdups.
A removed comment that says "We have to free the top level index
memory otherwise we would leak", and this will be addressed in the
next commit.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15590
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit ed7bc50b00ea62a12d17e56a686053e0a956d8f8
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed Jul 10 11:52:39 2024 +1200
ldb:ldb_kv_dn_list_find_val: check for int overflow
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit d8c7768fed51a764cef006a3497c709dd7ed6e15
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Tue Jul 9 14:47:25 2024 +1200
ldb_kv_cache: always initialise dn_list.strict
The strict flag is only read in list intersection, so most of the time
it doesn't matter whether it is set because that path is not used.
Nevertheless seeing it set to all kinds of values is distracting.
The undefined behaviour has likely been hidden from static analysis
because the structure is passed through the in-memory tdb before use.
Incorrect true values will have disabled an optimisation but not
caused the wrong result.
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit 351636efa7f1a35c2fe554237869b2f4e502915f
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed Jul 10 16:04:27 2024 +1200
tdb: allow tracing of internal tdb
This will trace internal databases to files like this:
tdb_0x5da896b51870.trace.267290
We avoid strlen(name) because name could be NULL in this case (which
works fine with glibc but feels bad).
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit e653b0870fd5d5684d90497565efc8463a72192a
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed Jul 10 14:35:28 2024 +1200
tdb: fix compilation with TDB_TRACE=1
../../lib/tdb/common/tdb.c: In function ‘tdb_trace_record’:
../../lib/tdb/common/tdb.c:1224:22: error: ‘snprintf’ output truncated before the last format character [-Werror=format-truncation=]
1224 | p += snprintf(p, 2, %02x, rec.dptr[i]);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/tdb/common/tdb.c:1224:22: note: ‘snprintf’ output 3 bytes into a destination of size 2
cc1: all warnings being treated as errors
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
-----------------------------------------------------------------------
Summary of changes:
lib/ldb/ldb_key_value/ldb_kv_index.c | 137 ++++++++++++++++++++---------------
lib/tdb/common/open.c | 21 ++++--
lib/tdb/common/tdb.c | 9 ++-
3 files changed, 96 insertions(+), 71 deletions(-)
Changeset truncated at 500 lines:
diff --git a/lib/ldb/ldb_key_value/ldb_kv_index.c b/lib/ldb/ldb_key_value/ldb_kv_index.c
index 3f1a847f2b6..ff8c04af015 100644
--- a/lib/ldb/ldb_key_value/ldb_kv_index.c
+++ b/lib/ldb/ldb_key_value/ldb_kv_index.c
@@ -275,6 +275,10 @@ static int ldb_kv_dn_list_find_val(struct ldb_kv_private *ldb_kv,
unsigned int i;
struct ldb_val *exact = NULL, *next = NULL;
+ if (unlikely(list->count > INT_MAX)) {
+ return -1;
+ }
+
if (ldb_kv->cache->GUID_index_attribute == NULL) {
for (i=0; i<list->count; i++) {
if (ldb_val_equal_exact(&list->dn[i], v) == 1) {
@@ -375,10 +379,7 @@ static int ldb_kv_dn_list_load(struct ldb_module *module,
bool from_primary_cache = false;
TDB_DATA key = {0};
- list->dn = NULL;
- list->count = 0;
- list->strict = false;
-
+ *list = (struct dn_list){};
/*
* See if we have an in memory index cache
*/
@@ -446,34 +447,39 @@ static int ldb_kv_dn_list_load(struct ldb_module *module,
* There is an active index sub transaction, and the record was
* found in the primary index transaction cache. A copy of the
* record needs be taken to prevent the original entry being
- * altered, until the index sub transaction is committed.
+ * altered, until the index sub transaction is committed, but we
+ * don't copy the actual values, just the array of struct ldb_val
+ * that points to the values (which are offsets into a GUID array).
+ *
+ * As a reminder, our primary cache is an in-memory tdb that
+ * maps attributes to struct dn_list objects, which point to
+ * the actual index, which is an array of struct ldb_val, the
+ * contents of which are {.data = <binary GUID>, .length =
+ * 16}. The array is sorted by GUID data, and these GUIDs are
+ * used to look up index entries in the main database. There
+ * are more layers of indirection than necessary, but what
+ * makes the index useful is we can use a binary search to
+ * find if the array contains a GUID.
+ *
+ * What we do in a sub-transaction is make a copy of the struct
+ * dn_list and the array of struct ldb_val, but *not* of the
+ * .data that they point to. This copy is put into a new
+ * in-memory tdb which masks the primary cache for the duration
+ * of the sub-transaction.
+ *
+ * In an add operation in a sub-transaction, the new ldb_val
+ * is a child of the sub-transaction dn_list, which will
+ * become the main dn_list if the transaction succeeds.
+ *
+ * These acrobatics do not affect read-only operations.
*/
-
- {
- struct ldb_val *dns = NULL;
- size_t x = 0;
-
- dns = talloc_array(
- list,
- struct ldb_val,
- list2->count);
- if (dns == NULL) {
- return LDB_ERR_OPERATIONS_ERROR;
- }
- for (x = 0; x < list2->count; x++) {
- dns[x].length = list2->dn[x].length;
- dns[x].data = talloc_memdup(
- dns,
- list2->dn[x].data,
- list2->dn[x].length);
- if (dns[x].data == NULL) {
- TALLOC_FREE(dns);
- return LDB_ERR_OPERATIONS_ERROR;
- }
- }
- list->dn = dns;
- list->count = list2->count;
+ list->dn = talloc_memdup(list,
+ list2->dn,
+ talloc_get_size(list2->dn));
+ if (list->dn == NULL) {
+ return LDB_ERR_OPERATIONS_ERROR;
}
+ list->count = list2->count;
return LDB_SUCCESS;
/*
@@ -593,6 +599,7 @@ int ldb_kv_key_dn_from_idx(struct ldb_module *module,
ldb_oom(ldb);
return LDB_ERR_OPERATIONS_ERROR;
}
+ list->strict = false;
ret = ldb_kv_index_dn_base_dn(module, ldb_kv, dn, list, &truncation);
if (ret != LDB_SUCCESS) {
@@ -863,6 +870,7 @@ static int ldb_kv_dn_list_store(struct ldb_module *module,
}
list2->dn = talloc_steal(list2, list->dn);
list2->count = list->count;
+ list2->strict = false;
rec.dptr = (uint8_t *)&list2;
rec.dsize = sizeof(void *);
@@ -962,10 +970,7 @@ int ldb_kv_index_transaction_cancel(struct ldb_module *module)
tdb_close(ldb_kv->idxptr->itdb);
}
TALLOC_FREE(ldb_kv->idxptr);
- if (ldb_kv->nested_idx_ptr && ldb_kv->nested_idx_ptr->itdb) {
- tdb_close(ldb_kv->nested_idx_ptr->itdb);
- }
- TALLOC_FREE(ldb_kv->nested_idx_ptr);
+ ldb_kv_index_sub_transaction_cancel(ldb_kv);
return LDB_SUCCESS;
}
@@ -1261,8 +1266,7 @@ static int ldb_kv_index_dn_simple(struct ldb_module *module,
ldb = ldb_module_get_ctx(module);
- list->count = 0;
- list->dn = NULL;
+ *list = (struct dn_list){};
/* if the attribute isn't in the list of indexed attributes then
this node needs a full search */
@@ -1312,17 +1316,14 @@ static int ldb_kv_index_dn_leaf(struct ldb_module *module,
const struct ldb_parse_tree *tree,
struct dn_list *list)
{
+ *list = (struct dn_list){};
if (ldb_kv->disallow_dn_filter &&
(ldb_attr_cmp(tree->u.equality.attr, "dn") == 0)) {
/* in AD mode we do not support "(dn=...)" search filters */
- list->dn = NULL;
- list->count = 0;
return LDB_SUCCESS;
}
if (tree->u.equality.attr[0] == '@') {
/* Do not allow a indexed search against an @ */
- list->dn = NULL;
- list->count = 0;
return LDB_SUCCESS;
}
if (ldb_attr_dn(tree->u.equality.attr) == 0) {
@@ -1334,16 +1335,12 @@ static int ldb_kv_index_dn_leaf(struct ldb_module *module,
&tree->u.equality.value);
if (dn == NULL) {
/* If we can't parse it, no match */
- list->dn = NULL;
- list->count = 0;
return LDB_SUCCESS;
}
valid_dn = ldb_dn_validate(dn);
if (valid_dn == false) {
/* If we can't parse it, no match */
- list->dn = NULL;
- list->count = 0;
return LDB_SUCCESS;
}
@@ -1452,8 +1449,7 @@ static bool list_intersect(struct ldb_kv_private *ldb_kv,
return false;
}
- list3->dn = talloc_array(list3, struct ldb_val,
- MIN(list->count, list2->count));
+ list3->dn = talloc_array(list3, struct ldb_val, short_list->count);
if (!list3->dn) {
talloc_free(list3);
return false;
@@ -3450,6 +3446,7 @@ static int delete_index(struct ldb_kv_private *ldb_kv,
* index entry */
list.dn = NULL;
list.count = 0;
+ list.strict = false;
/* the offset of 3 is to remove the DN= prefix. */
v.data = key.data + 3;
@@ -3852,9 +3849,7 @@ int ldb_kv_reindex(struct ldb_module *module)
* Copy the contents of the nested transaction index cache record to the
* transaction index cache.
*
- * During this 'commit' of the subtransaction to the main transaction
- * (cache), care must be taken to free any existing index at the top
- * level because otherwise we would leak memory.
+ * This is a 'commit' of the subtransaction to the main transaction cache.
*/
static int ldb_kv_sub_transaction_traverse(
struct tdb_context *tdb,
@@ -3883,8 +3878,7 @@ static int ldb_kv_sub_transaction_traverse(
/*
* Do we already have an entry in the primary transaction cache
- * If so free it's dn_list and replace it with the dn_list from
- * the secondary cache
+ * If so replace dn_list with the one from the subtransaction.
*
* The TDB and so the fetched rec contains NO DATA, just a
* pointer to data held in memory.
@@ -3897,21 +3891,41 @@ static int ldb_kv_sub_transaction_traverse(
abort();
}
/*
- * We had this key at the top level. However we made a copy
- * at the sub-transaction level so that we could possibly
- * roll back. We have to free the top level index memory
- * otherwise we would leak
+ * We had this key at the top level, and made a copy
+ * of the dn list for this sub-transaction level that
+ * borrowed the top level GUID data. We can't free the
+ * original dn list just yet.
+ *
+ * In this diagram, ... is the C pointer structure
+ * and --- is the talloc structure (::: is both).
+ *
+ * index_in_top_level ::: dn orig ..............
+ * | | :
+ * | `--GUID array :
+ * | |----- val1 data
+ * ldb_kv `----- val2 data
+ * | :
+ * index_in_subtransaction :: dn copy ..........:
+ * | :
+ * `------------ new val3 data
+ *
+ * So we don't free the index_in_top_level dn list yet,
+ * because we are (probably) borrowing most of its
+ * children. But we can save memory by discarding the
+ * values and keeping it as an almost empty talloc
+ * node.
*/
- if (index_in_top_level->count > 0) {
- TALLOC_FREE(index_in_top_level->dn);
- }
+ talloc_realloc(index_in_top_level,
+ index_in_top_level->dn, struct ldb_val *, 1);
index_in_top_level->dn
= talloc_steal(index_in_top_level,
index_in_subtransaction->dn);
index_in_top_level->count = index_in_subtransaction->count;
return 0;
}
-
+ /*
+ * We found no top level index in the cache, so we put one in.
+ */
index_in_top_level = talloc(ldb_kv->idxptr, struct dn_list);
if (index_in_top_level == NULL) {
ldb_kv->idxptr->error = LDB_ERR_OPERATIONS_ERROR;
@@ -3921,6 +3935,7 @@ static int ldb_kv_sub_transaction_traverse(
= talloc_steal(index_in_top_level,
index_in_subtransaction->dn);
index_in_top_level->count = index_in_subtransaction->count;
+ index_in_top_level->strict = false;
rec.dptr = (uint8_t *)&index_in_top_level;
rec.dsize = sizeof(void *);
@@ -3973,7 +3988,9 @@ int ldb_kv_index_sub_transaction_start(struct ldb_kv_private *ldb_kv)
int ldb_kv_index_sub_transaction_cancel(struct ldb_kv_private *ldb_kv)
{
if (ldb_kv->nested_idx_ptr != NULL) {
- tdb_close(ldb_kv->nested_idx_ptr->itdb);
+ if (ldb_kv->nested_idx_ptr->itdb != NULL) {
+ tdb_close(ldb_kv->nested_idx_ptr->itdb);
+ }
TALLOC_FREE(ldb_kv->nested_idx_ptr);
}
return LDB_SUCCESS;
diff --git a/lib/tdb/common/open.c b/lib/tdb/common/open.c
index 3fa7ce1389d..87312f7dc09 100644
--- a/lib/tdb/common/open.c
+++ b/lib/tdb/common/open.c
@@ -721,12 +721,21 @@ _PUBLIC_ struct tdb_context *tdb_open_ex(const char *name, int hash_size, int td
goto fail;
}
+ internal:
+ /* Internal (memory-only) databases skip all the code above to
+ * do with disk files, and resume here by releasing their
+ * open lock and hooking into the active list. */
+
#ifdef TDB_TRACE
{
- char tracefile[strlen(name) + 32];
-
- snprintf(tracefile, sizeof(tracefile),
- "%s.trace.%li", name, (long)getpid());
+ char tracefile[64];
+ if (tdb->flags & TDB_INTERNAL) {
+ snprintf(tracefile, sizeof(tracefile),
+ "tdb_%p.trace.%li", tdb, (long)getpid());
+ } else {
+ snprintf(tracefile, sizeof(tracefile),
+ "%s.trace.%li", name, (long)getpid());
+ }
tdb->tracefd = open(tracefile, O_WRONLY|O_CREAT|O_EXCL, 0600);
if (tdb->tracefd >= 0) {
tdb_enable_seqnum(tdb);
@@ -737,10 +746,6 @@ _PUBLIC_ struct tdb_context *tdb_open_ex(const char *name, int hash_size, int td
}
#endif
- internal:
- /* Internal (memory-only) databases skip all the code above to
- * do with disk files, and resume here by releasing their
- * open lock and hooking into the active list. */
if (tdb_nest_unlock(tdb, OPEN_LOCK, F_WRLCK, false) == -1) {
goto fail;
}
diff --git a/lib/tdb/common/tdb.c b/lib/tdb/common/tdb.c
index de829bb48c4..4ad01b2b360 100644
--- a/lib/tdb/common/tdb.c
+++ b/lib/tdb/common/tdb.c
@@ -1208,7 +1208,7 @@ static void tdb_trace_end_ret(struct tdb_context *tdb, int ret)
static void tdb_trace_record(struct tdb_context *tdb, TDB_DATA rec)
{
- char msg[20 + rec.dsize*2], *p;
+ char msg[21 + rec.dsize*2], *p;
unsigned int i;
/* We differentiate zero-length records from non-existent ones. */
@@ -1220,8 +1220,11 @@ static void tdb_trace_record(struct tdb_context *tdb, TDB_DATA rec)
/* snprintf here is purely cargo-cult programming. */
p = msg;
p += snprintf(p, sizeof(msg), " %zu:", rec.dsize);
- for (i = 0; i < rec.dsize; i++)
- p += snprintf(p, 2, "%02x", rec.dptr[i]);
+
+ for (i = 0; i < rec.dsize; i++) {
+ snprintf(p, 3, "%02x", rec.dptr[i]);
+ p += 2;
+ }
tdb_trace_write(tdb, msg);
}
--
Samba Shared Repository
More information about the samba-cvs
mailing list