[SCM] Samba Shared Repository - branch v4-20-test updated
Jule Anger
janger at samba.org
Mon Oct 7 09:46:02 UTC 2024
The branch, v4-20-test has been updated
via 44378caeb4c netcmd:domain:policy: Fix missing conversion from tgt_lifetime minutes to 10^(-7) seconds
via 0a99463b3e0 ldb:kv_index: help static analysers to not worry (CID 1615192)
via 76e1024f4c2 ldb:kv_index: realloc away old dn list
via 226b0a20bd1 ldb_kv_index: dn_list load sub transaction can re-use keys
from 676ac1793a1 s3: SIGHUP handlers use consistent log level 3
https://git.samba.org/?p=samba.git;a=shortlog;h=v4-20-test
- Log -----------------------------------------------------------------
commit 44378caeb4cd314d5f35f706835794fda179c0b4
Author: Andréas Leroux <aleroux at tranquil.it>
Date: Wed Sep 25 14:42:25 2024 +0200
netcmd:domain:policy: Fix missing conversion from tgt_lifetime minutes to 10^(-7) seconds
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15692
Signed-off-by: Andréas Leroux <aleroux at tranquil.it>
Reviewed-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Jennifer Sutton <jennifersutton at catalyst.net.nz>
Autobuild-User(master): Douglas Bagnall <dbagnall at samba.org>
Autobuild-Date(master): Fri Oct 4 04:01:22 UTC 2024 on atb-devel-224
(backported from commit 3766b6a126f659a43e2e36c66689c136fc22dbc4
requiring manual merge in the test file imports)
Autobuild-User(v4-20-test): Jule Anger <janger at samba.org>
Autobuild-Date(v4-20-test): Mon Oct 7 09:45:40 UTC 2024 on atb-devel-224
commit 0a99463b3e00dc5bda2759b7c8fa6e334899fe55
Author: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed Jul 31 09:20:50 2024 +1200
ldb:kv_index: help static analysers to not worry (CID 1615192)
The point of this realloc is that we are not using this array, but
keeping it around to remain a node the talloc tree. We'd prefer to
reduce it to nothing.
Coverity rightly spotted that it was reallocing an array of `struct
ldb_val` to an array of `struct ldb_val *`, which has a different size
and all. But it doesn't matter in this case, because we will never use
it.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15590
Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Jennifer Sutton <josutton at catalyst.net.nz>
(cherry picked from commit e2a74963fb89f5409c236a0fbe4cd070e1a75a43)
commit 76e1024f4c2852caedf5e7bfe5b132c90b2ac5b2
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>
(cherry picked from commit 1bf9ede94f0a6b41fb18e880e59a8e390f8c21d3)
commit 226b0a20bd1272adb958bd83ede03922f8c182e6
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>
(cherry picked from commit 5f0198d69843c864f2b98a7c0c6305ad789a68a0)
-----------------------------------------------------------------------
Summary of changes:
lib/ldb/ldb_key_value/ldb_kv_index.c | 100 +++++++++++++--------
python/samba/netcmd/domain/auth/policy.py | 18 ++--
.../samba/tests/samba_tool/domain_auth_policy.py | 19 ++--
3 files changed, 86 insertions(+), 51 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..0e706366872 100644
--- a/lib/ldb/ldb_key_value/ldb_kv_index.c
+++ b/lib/ldb/ldb_key_value/ldb_kv_index.c
@@ -446,34 +446,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;
/*
@@ -3852,9 +3857,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 +3886,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 +3899,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;
diff --git a/python/samba/netcmd/domain/auth/policy.py b/python/samba/netcmd/domain/auth/policy.py
index de9ce4b004f..21f083de147 100644
--- a/python/samba/netcmd/domain/auth/policy.py
+++ b/python/samba/netcmd/domain/auth/policy.py
@@ -28,7 +28,13 @@ from samba.netcmd.domain.models.auth_policy import MIN_TGT_LIFETIME,\
MAX_TGT_LIFETIME, StrongNTLMPolicy
from samba.netcmd.domain.models.exceptions import ModelError
from samba.netcmd.validators import Range
+from samba.nt_time import NT_TICKS_PER_SEC
+def mins_to_tgt_lifetime(minutes):
+ """Convert minutes to the tgt_lifetime attributes unit which is 10^-7 seconds"""
+ if minutes is not None:
+ return minutes * 60 * NT_TICKS_PER_SEC
+ return minutes
def check_similar_args(option, args):
"""Helper method for checking similar mutually exclusive args.
@@ -385,14 +391,14 @@ class cmd_domain_auth_policy_create(Command):
description=description,
strong_ntlm_policy=StrongNTLMPolicy[strong_ntlm_policy.upper()],
user_allow_ntlm_auth=useropts.allow_ntlm_auth,
- user_tgt_lifetime=useropts.tgt_lifetime,
+ user_tgt_lifetime=mins_to_tgt_lifetime(useropts.tgt_lifetime),
user_allowed_to_authenticate_from=useropts.allowed_to_authenticate_from,
user_allowed_to_authenticate_to=useropts.allowed_to_authenticate_to,
service_allow_ntlm_auth=serviceopts.allow_ntlm_auth,
- service_tgt_lifetime=serviceopts.tgt_lifetime,
+ service_tgt_lifetime=mins_to_tgt_lifetime(serviceopts.tgt_lifetime),
service_allowed_to_authenticate_from=serviceopts.allowed_to_authenticate_from,
service_allowed_to_authenticate_to=serviceopts.allowed_to_authenticate_to,
- computer_tgt_lifetime=computeropts.tgt_lifetime,
+ computer_tgt_lifetime=mins_to_tgt_lifetime(computeropts.tgt_lifetime),
computer_allowed_to_authenticate_to=computeropts.allowed_to_authenticate_to,
)
@@ -575,7 +581,7 @@ class cmd_domain_auth_policy_modify(Command):
StrongNTLMPolicy[strong_ntlm_policy.upper()]
if useropts.tgt_lifetime is not None:
- policy.user_tgt_lifetime = useropts.tgt_lifetime
+ policy.user_tgt_lifetime = mins_to_tgt_lifetime(useropts.tgt_lifetime)
if useropts.allowed_to_authenticate_from is not None:
policy.user_allowed_to_authenticate_from = \
@@ -589,7 +595,7 @@ class cmd_domain_auth_policy_modify(Command):
##################
if serviceopts.tgt_lifetime is not None:
- policy.service_tgt_lifetime = serviceopts.tgt_lifetime
+ policy.service_tgt_lifetime = mins_to_tgt_lifetime(serviceopts.tgt_lifetime)
if serviceopts.allowed_to_authenticate_from is not None:
policy.service_allowed_to_authenticate_from = \
@@ -603,7 +609,7 @@ class cmd_domain_auth_policy_modify(Command):
###########
if computeropts.tgt_lifetime is not None:
- policy.computer_tgt_lifetime = computeropts.tgt_lifetime
+ policy.computer_tgt_lifetime = mins_to_tgt_lifetime(computeropts.tgt_lifetime)
if computeropts.allowed_to_authenticate_to is not None:
policy.computer_allowed_to_authenticate_to = \
diff --git a/python/samba/tests/samba_tool/domain_auth_policy.py b/python/samba/tests/samba_tool/domain_auth_policy.py
index 1854037dd3a..dcc732d177b 100644
--- a/python/samba/tests/samba_tool/domain_auth_policy.py
+++ b/python/samba/tests/samba_tool/domain_auth_policy.py
@@ -27,12 +27,19 @@ from unittest.mock import patch
from samba.dcerpc import security
from samba.ndr import ndr_pack, ndr_unpack
from samba.netcmd.domain.models.exceptions import ModelError
+from samba.nt_time import NT_TICKS_PER_SEC
from samba.samdb import SamDB
from samba.sd_utils import SDUtils
from .silo_base import SiloTest
+def mins_to_tgt_lifetime(minutes):
+ """Convert minutes to the tgt_lifetime attributes unit which is 10^-7 seconds"""
+ if minutes is not None:
+ return minutes * 60 * NT_TICKS_PER_SEC
+ return minutes
+
class AuthPolicyCmdTestCase(SiloTest):
def test_list(self):
@@ -135,7 +142,7 @@ class AuthPolicyCmdTestCase(SiloTest):
# Check policy fields.
policy = self.get_authentication_policy(name)
self.assertEqual(str(policy["cn"]), name)
- self.assertEqual(str(policy["msDS-UserTGTLifetime"]), "60")
+ self.assertEqual(str(policy["msDS-UserTGTLifetime"]), str(mins_to_tgt_lifetime(60)))
# check lower bounds (45)
result, out, err = self.runcmd("domain", "auth", "policy", "create",
@@ -254,7 +261,7 @@ class AuthPolicyCmdTestCase(SiloTest):
# Check policy fields.
policy = self.get_authentication_policy(name)
self.assertEqual(str(policy["cn"]), name)
- self.assertEqual(str(policy["msDS-ServiceTGTLifetime"]), "60")
+ self.assertEqual(str(policy["msDS-ServiceTGTLifetime"]), str(mins_to_tgt_lifetime(60)))
# check lower bounds (45)
result, out, err = self.runcmd("domain", "auth", "policy", "create",
@@ -373,7 +380,7 @@ class AuthPolicyCmdTestCase(SiloTest):
# Check policy fields.
policy = self.get_authentication_policy(name)
self.assertEqual(str(policy["cn"]), name)
- self.assertEqual(str(policy["msDS-ComputerTGTLifetime"]), "60")
+ self.assertEqual(str(policy["msDS-ComputerTGTLifetime"]), str(mins_to_tgt_lifetime(60)))
# check lower bounds (45)
result, out, err = self.runcmd("domain", "auth", "policy", "create",
@@ -840,7 +847,7 @@ class AuthPolicyCmdTestCase(SiloTest):
# Verify field was changed.
policy = self.get_authentication_policy(name)
- self.assertEqual(str(policy["msDS-UserTGTLifetime"]), "120")
+ self.assertEqual(str(policy["msDS-UserTGTLifetime"]), str(mins_to_tgt_lifetime(120)))
# check lower bounds (45)
result, out, err = self.runcmd("domain", "auth", "policy", "modify",
@@ -876,7 +883,7 @@ class AuthPolicyCmdTestCase(SiloTest):
# Verify field was changed.
policy = self.get_authentication_policy(name)
- self.assertEqual(str(policy["msDS-ServiceTGTLifetime"]), "120")
+ self.assertEqual(str(policy["msDS-ServiceTGTLifetime"]), str(mins_to_tgt_lifetime(120)))
# check lower bounds (45)
result, out, err = self.runcmd("domain", "auth", "policy", "modify",
@@ -912,7 +919,7 @@ class AuthPolicyCmdTestCase(SiloTest):
# Verify field was changed.
policy = self.get_authentication_policy(name)
- self.assertEqual(str(policy["msDS-ComputerTGTLifetime"]), "120")
+ self.assertEqual(str(policy["msDS-ComputerTGTLifetime"]), str(mins_to_tgt_lifetime(120)))
# check lower bounds (45)
result, out, err = self.runcmd("domain", "auth", "policy", "modify",
--
Samba Shared Repository
More information about the samba-cvs
mailing list