Attachment 16774 Details for Bug 14821 – patch for 4.15, 4.14, 4.13

[patch] patch for 4.15, 4.14, 4.13

cpu-overuse.patch (text/plain), 33.59 KB, created by Andrew Bartlett on 2021-09-06 03:15:32 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Andrew Bartlett

Created: 2021-09-06 03:15:32 UTC

Size: 33.59 KB

patch

obsolete

>From 21b7cec6993b81227e17e43ebb32c095e0bd20ed Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Sat, 4 Sep 2021 12:28:20 +1200
>Subject: [PATCH 1/6] selftest: Split up targets for samba_tool_drs from
> samba_tool_drs_showrepl
>
>These now run in the disconnected sets schema_dc/schema_pair_dc and
>ad_dc/vampire_dc/promoted_dc.  By aiming at different sets ofservers
>we can't cause cross-contamination in terms of which servers are
>listed as outbound connections.
>
>Also, by running the tests only once we reduce the chaces of trouble
>by half.
>
>RN: Address flapping samba_tool_drs_showrepl test
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14818
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>(cherry picked from commit e8b4599e0935290c5e59df9fd4f695ad8d6f361c)
>---
> source4/selftest/tests.py | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
>diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py
>index 8938754d0fc..a011d50a768 100755
>--- a/source4/selftest/tests.py
>+++ b/source4/selftest/tests.py
>@@ -1295,6 +1295,18 @@ planoldpythontestsuite(env, "ridalloc_exop",
>                        environ={'DC1': "$DC_SERVER", 'DC2': '$SERVER'},
>                        extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD'])
> 
>+# This test can pollute the environment a little by creating and
>+# deleting DCs which can get into the replication state for a while.
>+#
>+# The setting of DC1 to $DC_SERVER means that it will join towards and
>+# operate on schema_dc.  This matters most when running
>+# test_samba_tool_replicate_local as this sets up a full temp DC and
>+# does new replication to it, which can show up in the replication
>+# topology.
>+#
>+# That is why this test is run on the isolated environment and not on
>+# those connected with ad_dc (vampiredc/promoteddc)
>+
> env = 'schema_pair_dc'
> planoldpythontestsuite("%s:local" % env, "samba_tool_drs",
>                        extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')],
>@@ -1309,11 +1321,6 @@ planoldpythontestsuite(env, "getnc_schema",
>                        extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD'])
> 
> for env in ['vampire_dc', 'promoted_dc']:
>-    planoldpythontestsuite("%s:local" % env, "samba_tool_drs",
>-                           extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')],
>-                           name="samba4.drs.samba_tool_drs.python(%s)" % env,
>-                           environ={'DC1': '$DC_SERVER', 'DC2': '$SERVER'},
>-                           extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD'])
>     planoldpythontestsuite("%s:local" % env, "samba_tool_drs_showrepl",
>                            extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')],
>                            name="samba4.drs.samba_tool_drs_showrepl.python(%s)" % env,
>-- 
>2.25.1
>
>
>From 46ad637584149328e60463aec65593c42bcdc602 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Sat, 4 Sep 2021 13:11:08 +1200
>Subject: [PATCH 2/6] selftest: Only run samba_tool_drs_showrepl test once
>
>This test is not slow, but there is no value running it twice.
>
>Running this test twice just increases the chances we might
>loose a race as it shows and validates live replication data.
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>(cherry picked from commit 75a5ed66731e947fa16af81aab7649d1fddec45f)
>---
> source4/selftest/tests.py | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
>diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py
>index a011d50a768..32059461831 100755
>--- a/source4/selftest/tests.py
>+++ b/source4/selftest/tests.py
>@@ -1320,12 +1320,18 @@ planoldpythontestsuite(env, "getnc_schema",
>                                 "PLEASE_BREAK_MY_WINDOWS": "1"},
>                        extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD'])
> 
>+# This test can be sensitive to the DC joins and replications don in
>+# "samba_tool_drs" so run this is run against scheam_pair_dc/schema_dc
>+# not the set of environments connected with ad_dc.
>+
>+# This will show the replication state of ad_dc
>+planoldpythontestsuite("promoted_dc:local", "samba_tool_drs_showrepl",
>+                       extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')],
>+                       name="samba4.drs.samba_tool_drs_showrepl.python(%s)" % env,
>+                       environ={'DC1': '$DC_SERVER', 'DC2': '$SERVER'},
>+                       extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD'])
>+
> for env in ['vampire_dc', 'promoted_dc']:
>-    planoldpythontestsuite("%s:local" % env, "samba_tool_drs_showrepl",
>-                           extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')],
>-                           name="samba4.drs.samba_tool_drs_showrepl.python(%s)" % env,
>-                           environ={'DC1': '$DC_SERVER', 'DC2': '$SERVER'},
>-                           extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD'])
>     planoldpythontestsuite("%s:local" % env, "replica_sync",
>                            extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')],
>                            name="samba4.drs.replica_sync.python(%s)" % env,
>-- 
>2.25.1
>
>
>From e1cef55699cc912c932fc3198f0eec7ed78e8a3d Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 25 Aug 2021 09:41:11 +1200
>Subject: [PATCH 3/6] dsdb: Be careful to avoid use of the expensive
> talloc_is_parent()
>
>The wrong talloc API was selected while addressing a memory leak.
>
>commit ee2fe56ba0ef6626b634376e8dc2185aa89f8c99
>Author: Aaron Haslett <aaronhaslett@catalyst.net.nz>
>Date:   Tue Nov 27 11:07:44 2018 +1300
>
>    drepl: memory leak fix
>
>    Fixes a memory leak where schema reference attached to ldb
>    instance is lost before it can be freed.
>
>    BUG: https://bugzilla.samba.org/show_bug.cgi?id=14042
>
>    Signed-off-by: Aaron Haslett <aaronhaslett@catalyst.net.nz>
>
>    Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>    Reviewed-by: Garming Sam <garming@catalyst.net.nz>
>
>    Autobuild-User(master): Garming Sam <garming@samba.org>
>    Autobuild-Date(master): Wed Jul 17 06:17:10 UTC 2019 on sn-devel-184
>
>By using talloc_get_parent() walking the entire talloc tree is
>avoided.
>
>RN: Address a signifcant performance regression in database access in the AD DC since Samba 4.12
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14806
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>(cherry picked from commit 8affe4a1e625104de4ca024fdc3e9cd96498aff3)
>---
> source4/dsdb/schema/schema_set.c | 41 ++++++++++++++++++++++----------
> 1 file changed, 29 insertions(+), 12 deletions(-)
>
>diff --git a/source4/dsdb/schema/schema_set.c b/source4/dsdb/schema/schema_set.c
>index f4934917c7c..45faa0912ec 100644
>--- a/source4/dsdb/schema/schema_set.c
>+++ b/source4/dsdb/schema/schema_set.c
>@@ -698,6 +698,8 @@ int dsdb_reference_schema(struct ldb_context *ldb, struct dsdb_schema *schema,
> {
> 	int ret;
> 	void *ptr;
>+	void *schema_parent = NULL;
>+	bool is_already_parent;
> 	struct dsdb_schema *old_schema;
> 	old_schema = ldb_get_opaque(ldb, "dsdb_schema");
> 	ret = ldb_set_opaque(ldb, "dsdb_schema", schema);
>@@ -710,8 +712,9 @@ int dsdb_reference_schema(struct ldb_context *ldb, struct dsdb_schema *schema,
> 	talloc_unlink(ldb, old_schema);
> 
> 	/* Reference schema on ldb if it wasn't done already */
>-	ret = talloc_is_parent(ldb, schema);
>-	if (ret == 0) {
>+	schema_parent = talloc_parent(schema);
>+	is_already_parent = (schema_parent == ldb);
>+	if (!is_already_parent) {
> 		ptr = talloc_reference(ldb, schema);
> 		if (ptr == NULL) {
> 			return ldb_oom(ldb);
>@@ -775,10 +778,10 @@ int dsdb_set_global_schema(struct ldb_context *ldb)
> 	/* 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) {
>-		/* If ldb doesn't have a reference to the schema, make one,
>-		 * just in case the original copy is replaced */
>-		ret = talloc_is_parent(ldb, global_schema);
>-		if (ret == 0) {
>+		void *schema_parent = talloc_parent(global_schema);
>+		bool is_already_parent =
>+			(schema_parent == ldb);
>+		if (!is_already_parent) {
> 			ptr = talloc_reference(ldb, global_schema);
> 			if (ptr == NULL) {
> 				return ldb_oom(ldb);
>@@ -809,7 +812,6 @@ struct dsdb_schema *dsdb_get_schema(struct ldb_context *ldb, TALLOC_CTX *referen
> 	dsdb_schema_refresh_fn refresh_fn;
> 	struct ldb_module *loaded_from_module;
> 	bool use_global_schema;
>-	int ret;
> 	TALLOC_CTX *tmp_ctx = talloc_new(reference_ctx);
> 	if (tmp_ctx == NULL) {
> 		return NULL;
>@@ -860,13 +862,28 @@ struct dsdb_schema *dsdb_get_schema(struct ldb_context *ldb, TALLOC_CTX *referen
> 	/* This removes the extra reference above */
> 	talloc_free(tmp_ctx);
> 
>-	/* If ref ctx exists and doesn't already reference schema, then add
>-	 * a reference.  Otherwise, just return schema.*/
>-	ret = talloc_is_parent(reference_ctx, schema_out);
>-	if ((ret == 1) || (!reference_ctx)) {
>+	/*
>+	 * If ref ctx exists and doesn't already reference schema, then add
>+	 * a reference.  Otherwise, just return schema.
>+	 *
>+	 * We must use talloc_parent(), which is not quite free (there
>+	 * is no direct parent pointer in talloc, only one on the
>+	 * first child within a linked list), but is much cheaper than
>+	 * talloc_is_parent() which walks the whole tree up to the top
>+	 * looking for a potential grand-grand(etc)-parent.
>+	 */
>+	if (reference_ctx == NULL) {
> 		return schema_out;
> 	} else {
>-		return talloc_reference(reference_ctx, schema_out);
>+		void *schema_parent = talloc_parent(schema_out);
>+		bool is_already_parent =
>+			schema_parent == reference_ctx;
>+		if (is_already_parent) {
>+			return schema_out;
>+		} else {
>+			return talloc_reference(reference_ctx,
>+						schema_out);
>+		}
> 	}
> }
> 
>-- 
>2.25.1
>
>
>From fa6f8b6e68037dded768efa735cea8c47d0e9daa Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 25 Aug 2021 09:54:04 +0000
>Subject: [PATCH 4/6] selftest: Add a test for LookupSids3 and LookupNames4 in
> python
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14807
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>(cherry picked from commit b40761b42e889369599c5eb355028ba377c43b49)
>---
> python/samba/tests/dcerpc/lsa.py | 333 +++++++++++++++++++++++++++++++
> source4/selftest/tests.py        |   1 +
> 2 files changed, 334 insertions(+)
> create mode 100644 python/samba/tests/dcerpc/lsa.py
>
>diff --git a/python/samba/tests/dcerpc/lsa.py b/python/samba/tests/dcerpc/lsa.py
>new file mode 100644
>index 00000000000..4377c42e9b8
>--- /dev/null
>+++ b/python/samba/tests/dcerpc/lsa.py
>@@ -0,0 +1,333 @@
>+# -*- coding: utf-8 -*-
>+#
>+# Unix SMB/CIFS implementation.
>+# Copyright Â© Andrew Bartlett <abartlet@samba.org> 2021
>+# Copyright (C) Catalyst IT Ltd. 2017
>+#
>+# This program is free software; you can redistribute it and/or modify
>+# it under the terms of the GNU General Public License as published by
>+# the Free Software Foundation; either version 3 of the License, or
>+# (at your option) any later version.
>+#
>+# This program is distributed in the hope that it will be useful,
>+# but WITHOUT ANY WARRANTY; without even the implied warranty of
>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+# GNU General Public License for more details.
>+#
>+# You should have received a copy of the GNU General Public License
>+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>+#
>+
>+"""Tests for samba.dcerpc.sam."""
>+
>+from samba.dcerpc import samr, security, lsa
>+from samba.credentials import Credentials
>+from samba.tests import TestCase
>+from samba.dcerpc.security import dom_sid
>+from samba import NTSTATUSError
>+from samba.ntstatus import NT_STATUS_ACCESS_DENIED
>+import samba.tests
>+
>+class LsaTests(TestCase):
>+
>+    def setUp(self):
>+        self.lp = self.get_loadparm()
>+        self.server = samba.tests.env_get_var_value('SERVER')
>+
>+    def test_lsa_LookupSids3_multiple(self):
>+        machine_creds = Credentials()
>+        machine_creds.guess(self.lp)
>+        machine_creds.set_machine_account()
>+
>+        c = lsa.lsarpc(
>+            "ncacn_ip_tcp:%s[schannel,seal]" % self.server,
>+            self.lp,
>+            machine_creds)
>+
>+        sids = lsa.SidArray()
>+        sid = lsa.SidPtr()
>+        # Need a set
>+        x = dom_sid("S-1-5-7")
>+        sid.sid = x
>+        sids.sids = [sid]
>+        sids.num_sids = 1
>+        names = lsa.TransNameArray2()
>+        level = lsa.LSA_LOOKUP_NAMES_ALL
>+        count = 0
>+        lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES
>+        client_revision = lsa.LSA_CLIENT_REVISION_2
>+
>+        # We want to run LookupSids3 multiple times on the same
>+        # connection as we have code to re-use the sam.ldb and we need
>+        # to check things work for the second request.
>+        (domains, names, count) = c.LookupSids3(sids, names, level, count, lookup_options, client_revision)
>+        self.assertEqual(count, 1)
>+        self.assertEqual(names.count, 1)
>+        self.assertEqual(names.names[0].name.string,
>+                         "ANONYMOUS LOGON")
>+        (domains2, names2, count2) = c.LookupSids3(sids, names, level, count, lookup_options, client_revision)
>+        self.assertEqual(count2, 1)
>+        self.assertEqual(names2.count, 1)
>+        self.assertEqual(names2.names[0].name.string,
>+                         "ANONYMOUS LOGON")
>+
>+        # Just looking for any exceptions in the last couple of loops
>+        c.LookupSids3(sids, names, level, count, lookup_options, client_revision)
>+        c.LookupSids3(sids, names, level, count, lookup_options, client_revision)
>+
>+    def test_lsa_LookupSids3_multiple_conns(self):
>+        machine_creds = Credentials()
>+        machine_creds.guess(self.lp)
>+        machine_creds.set_machine_account()
>+
>+        c = lsa.lsarpc(
>+            "ncacn_ip_tcp:%s[schannel,seal]" % self.server,
>+            self.lp,
>+            machine_creds)
>+
>+        sids = lsa.SidArray()
>+        sid = lsa.SidPtr()
>+        # Need a set
>+        x = dom_sid("S-1-5-7")
>+        sid.sid = x
>+        sids.sids = [sid]
>+        sids.num_sids = 1
>+        names = lsa.TransNameArray2()
>+        level = lsa.LSA_LOOKUP_NAMES_ALL
>+        count = 0
>+        lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES
>+        client_revision = lsa.LSA_CLIENT_REVISION_2
>+
>+        # We want to run LookupSids3, and then again on a new
>+        # connection to show that we don't have an issue with the DB
>+        # being tied to the wrong connection.
>+        (domains, names, count) = c.LookupSids3(sids,
>+                                                names,
>+                                                level,
>+                                                count,
>+                                                lookup_options,
>+                                                client_revision)
>+        self.assertEqual(count, 1)
>+        self.assertEqual(names.count, 1)
>+        self.assertEqual(names.names[0].name.string,
>+                         "ANONYMOUS LOGON")
>+
>+        c = lsa.lsarpc(
>+            "ncacn_ip_tcp:%s[schannel,seal]" % self.server,
>+            self.lp,
>+            machine_creds)
>+
>+        (domains, names, count) = c.LookupSids3(sids,
>+                                                names,
>+                                                level,
>+                                                count,
>+                                                lookup_options,
>+                                                client_revision)
>+        self.assertEqual(count, 1)
>+        self.assertEqual(names.count, 1)
>+        self.assertEqual(names.names[0].name.string,
>+                         "ANONYMOUS LOGON")
>+
>+
>+    def test_lsa_LookupNames4_LookupSids3_multiple(self):
>+        """
>+        Test by going back and forward between real DB lookups
>+        name->sid->name to ensure the sam.ldb handle is fine once
>+        shared
>+        """
>+
>+        machine_creds = Credentials()
>+        machine_creds.guess(self.lp)
>+        machine_creds.set_machine_account()
>+
>+        c_normal = lsa.lsarpc(
>+            "ncacn_np:%s[seal]" % self.server,
>+            self.lp,
>+            machine_creds)
>+
>+        username, domain = c_normal.GetUserName(None, None, None)
>+
>+        c = lsa.lsarpc(
>+            "ncacn_ip_tcp:%s[schannel,seal]" % self.server,
>+            self.lp,
>+            machine_creds)
>+
>+        sids  = lsa.TransSidArray3()
>+        names = [username]
>+        level = lsa.LSA_LOOKUP_NAMES_ALL
>+        count = 0
>+        lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES
>+        client_revision = lsa.LSA_CLIENT_REVISION_2
>+        (domains, sids, count) = c.LookupNames4(names,
>+                                                sids,
>+                                                level,
>+                                                count,
>+                                                lookup_options,
>+                                                client_revision)
>+
>+        # Another lookup on the same connection, will re-used the
>+        # server-side implicit state handle on the connection
>+        (domains, sids, count) = c.LookupNames4(names,
>+                                                sids,
>+                                                level,
>+                                                count,
>+                                                lookup_options,
>+                                                client_revision)
>+
>+        self.assertEqual(count, 1)
>+        self.assertEqual(sids.count, 1)
>+
>+        # Now look the SIDs back up
>+        names = lsa.TransNameArray2()
>+        sid = lsa.SidPtr()
>+        sid.sid = sids.sids[0].sid
>+        lookup_sids = lsa.SidArray()
>+        lookup_sids.sids = [sid]
>+        lookup_sids.num_sids = 1
>+        level = lsa.LSA_LOOKUP_NAMES_ALL
>+        count = 1
>+        lookup_options = 0
>+        client_revision = lsa.LSA_CLIENT_REVISION_2
>+
>+        (domains, names, count) = c.LookupSids3(lookup_sids,
>+                                                names,
>+                                                level,
>+                                                count,
>+                                                lookup_options,
>+                                                client_revision)
>+        self.assertEqual(count, 1)
>+        self.assertEqual(names.count, 1)
>+        self.assertEqual(names.names[0].name.string,
>+                         username.string)
>+
>+        # And once more just to be sure, just checking for a fault
>+        sids  = lsa.TransSidArray3()
>+        names = [username]
>+        level = lsa.LSA_LOOKUP_NAMES_ALL
>+        count = 0
>+        lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES
>+        client_revision = lsa.LSA_CLIENT_REVISION_2
>+        (domains, sids, count) = c.LookupNames4(names,
>+                                                sids,
>+                                                level,
>+                                                count,
>+                                                lookup_options,
>+                                                client_revision)
>+
>+
>+    def test_lsa_LookupNames4_multiple_conns(self):
>+        """
>+        Test by going back and forward between real DB lookups
>+        name->sid->name to ensure the sam.ldb handle is fine once
>+        shared
>+        """
>+
>+        machine_creds = Credentials()
>+        machine_creds.guess(self.lp)
>+        machine_creds.set_machine_account()
>+
>+        c_normal = lsa.lsarpc(
>+            "ncacn_np:%s[seal]" % self.server,
>+            self.lp,
>+            machine_creds)
>+
>+        username, domain = c_normal.GetUserName(None, None, None)
>+
>+        c = lsa.lsarpc(
>+            "ncacn_ip_tcp:%s[schannel,seal]" % self.server,
>+            self.lp,
>+            machine_creds)
>+
>+        sids  = lsa.TransSidArray3()
>+        names = [username]
>+        level = lsa.LSA_LOOKUP_NAMES_ALL
>+        count = 0
>+        lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES
>+        client_revision = lsa.LSA_CLIENT_REVISION_2
>+        (domains, sids, count) = c.LookupNames4(names,
>+                                                sids,
>+                                                level,
>+                                                count,
>+                                                lookup_options,
>+                                                client_revision)
>+
>+        c = lsa.lsarpc(
>+            "ncacn_ip_tcp:%s[schannel,seal]" % self.server,
>+            self.lp,
>+            machine_creds)
>+
>+        sids  = lsa.TransSidArray3()
>+        names = [username]
>+        level = lsa.LSA_LOOKUP_NAMES_ALL
>+        count = 0
>+        lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES
>+        client_revision = lsa.LSA_CLIENT_REVISION_2
>+        (domains, sids, count) = c.LookupNames4(names,
>+                                                sids,
>+                                                level,
>+                                                count,
>+                                                lookup_options,
>+                                                client_revision)
>+
>+    def test_lsa_LookupNames4_without_schannel(self):
>+
>+        machine_creds = Credentials()
>+        machine_creds.guess(self.lp)
>+        machine_creds.set_machine_account()
>+
>+        c_normal = lsa.lsarpc(
>+            "ncacn_np:%s[seal]" % self.server,
>+            self.lp,
>+            machine_creds)
>+
>+        username, domain = c_normal.GetUserName(None, None, None)
>+
>+        sids  = lsa.TransSidArray3()
>+        names = [username]
>+        level = lsa.LSA_LOOKUP_NAMES_ALL
>+        count = 0
>+        lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES
>+        client_revision = lsa.LSA_CLIENT_REVISION_2
>+
>+        with self.assertRaises(NTSTATUSError) as e:
>+            c_normal.LookupNames4(names,
>+                                  sids,
>+                                  level,
>+                                  count,
>+                                  lookup_options,
>+                                  client_revision)
>+        if (e.exception.args[0] != NT_STATUS_ACCESS_DENIED):
>+            raise AssertionError("LookupNames4 without schannel must fail with ACCESS_DENIED")
>+
>+    def test_lsa_LookupSids3_without_schannel(self):
>+        machine_creds = Credentials()
>+        machine_creds.guess(self.lp)
>+        machine_creds.set_machine_account()
>+
>+        c = lsa.lsarpc(
>+            "ncacn_ip_tcp:%s[seal]" % self.server,
>+            self.lp,
>+            machine_creds)
>+
>+        sids = lsa.SidArray()
>+        sid = lsa.SidPtr()
>+        # Need a set
>+        x = dom_sid("S-1-5-7")
>+        sid.sid = x
>+        sids.sids = [sid]
>+        sids.num_sids = 1
>+        names = lsa.TransNameArray2()
>+        level = lsa.LSA_LOOKUP_NAMES_ALL
>+        count = 0
>+        lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES
>+        client_revision = lsa.LSA_CLIENT_REVISION_2
>+
>+        with self.assertRaises(NTSTATUSError) as e:
>+            c.LookupSids3(sids,
>+                          names,
>+                          level,
>+                          count,
>+                          lookup_options,
>+                          client_revision)
>+        if (e.exception.args[0] != NT_STATUS_ACCESS_DENIED):
>+            raise AssertionError("LookupSids3 without schannel must fail with ACCESS_DENIED")
>diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py
>index 32059461831..638bd57334e 100755
>--- a/source4/selftest/tests.py
>+++ b/source4/selftest/tests.py
>@@ -770,6 +770,7 @@ planpythontestsuite("ad_dc_default:local", "samba.tests.dcerpc.sam")
> planpythontestsuite("ad_dc_default:local", "samba.tests.dsdb")
> planpythontestsuite("none", "samba.tests.dsdb_lock")
> planpythontestsuite("ad_dc_default:local", "samba.tests.dcerpc.bare")
>+planpythontestsuite("ad_dc_default:local", "samba.tests.dcerpc.lsa")
> planpythontestsuite("ad_dc_default:local", "samba.tests.dcerpc.unix")
> planpythontestsuite("ad_dc_ntvfs:local", "samba.tests.dcerpc.srvsvc")
> planpythontestsuite("ad_dc_default:local", "samba.tests.samba_tool.timecmd")
>-- 
>2.25.1
>
>
>From 3d88c735ee87311db27871550fe58fa676fe3122 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 25 Aug 2021 12:03:08 +1200
>Subject: [PATCH 5/6] s4-lsa: Cache sam.ldb handle in
> lsa_LookupSids3/LookupNames4
>
>Since 5c0345ea9bb34695dcd7be6c913748323bebe937 this
>would not have been implicitly cached via the ldb_wrap
>cache, due to the recording of the remote IP address
>(which is a good thing).
>
>This creates a more explicit and direct correct
>cache on the connection.
>
>The common code, including the SCHANNEL check is
>placed into a helper function.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14807
>
>RN: Fix performance regression in lsa_LookupSids3/LookupNames4 since Samba 4.9 by using an explicit database handle cache
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>
>Autobuild-User(master): Jeremy Allison <jra@samba.org>
>Autobuild-Date(master): Sun Sep  5 03:19:26 UTC 2021 on sn-devel-184
>
>(cherry picked from commit ae57d22e45b33537e9fca5969e9b68abd1ad633f)
>---
> source4/rpc_server/lsa/lsa_lookup.c | 131 ++++++++++++++++++++--------
> 1 file changed, 94 insertions(+), 37 deletions(-)
>
>diff --git a/source4/rpc_server/lsa/lsa_lookup.c b/source4/rpc_server/lsa/lsa_lookup.c
>index d41997d4b3d..61cb8a10a23 100644
>--- a/source4/rpc_server/lsa/lsa_lookup.c
>+++ b/source4/rpc_server/lsa/lsa_lookup.c
>@@ -663,25 +663,24 @@ NTSTATUS dcesrv_lsa_LookupSids2(struct dcesrv_call_state *dce_call,
> 	return status;
> }
> 
>+/* A random hexidecimal number (honest!) */
>+#define LSA_SERVER_IMPLICIT_POLICY_STATE_MAGIC 0xc0c99e00
> 
> /*
>-  lsa_LookupSids3
>-
>-  Identical to LookupSids2, but doesn't take a policy handle
>-  
>+  Ensure we're operating on an schannel connection,
>+  and use a lsa_policy_state cache on the connection.
> */
>-NTSTATUS dcesrv_lsa_LookupSids3(struct dcesrv_call_state *dce_call,
>-				TALLOC_CTX *mem_ctx,
>-				struct lsa_LookupSids3 *r)
>+static NTSTATUS schannel_call_setup(struct dcesrv_call_state *dce_call,
>+				    struct lsa_policy_state **_policy_state)
> {
>+	struct lsa_policy_state *policy_state = NULL;
> 	enum dcerpc_transport_t transport =
> 		dcerpc_binding_get_transport(dce_call->conn->endpoint->ep_description);
> 	enum dcerpc_AuthType auth_type = DCERPC_AUTH_TYPE_NONE;
>-	struct dcesrv_lsa_LookupSids_base_state *state = NULL;
>-	NTSTATUS status;
>-
> 	if (transport != NCACN_IP_TCP) {
>-		DCESRV_FAULT(DCERPC_FAULT_ACCESS_DENIED);
>+		/* We can't call DCESRV_FAULT() in the sub-function */
>+		dce_call->fault_code = DCERPC_FAULT_ACCESS_DENIED;
>+		return NT_STATUS_ACCESS_DENIED;
> 	}
> 
> 	/*
>@@ -693,8 +692,59 @@ NTSTATUS dcesrv_lsa_LookupSids3(struct dcesrv_call_state *dce_call,
> 	 */
> 	dcesrv_call_auth_info(dce_call, &auth_type, NULL);
> 	if (auth_type != DCERPC_AUTH_TYPE_SCHANNEL) {
>-		DCESRV_FAULT(DCERPC_FAULT_ACCESS_DENIED);
>+		/* We can't call DCESRV_FAULT() in the sub-function */
>+		dce_call->fault_code = DCERPC_FAULT_ACCESS_DENIED;
>+		return NT_STATUS_ACCESS_DENIED;
>+	}
>+
>+	/*
>+	 * We don't have a policy handle on this call, so we want to
>+	 * make a policy state and cache it for the life of the
>+	 * connection, to avoid re-opening the DB each call.
>+	 */
>+	policy_state
>+		= dcesrv_iface_state_find_conn(dce_call,
>+					       LSA_SERVER_IMPLICIT_POLICY_STATE_MAGIC,
>+					       struct lsa_policy_state);
>+
>+	if (policy_state == NULL) {
>+		NTSTATUS status
>+			= dcesrv_lsa_get_policy_state(dce_call,
>+						      dce_call /* mem_ctx */,
>+						      0, /* we skip access checks */
>+						      &policy_state);
>+		if (!NT_STATUS_IS_OK(status)) {
>+			return status;
>+		}
>+
>+		/*
>+		 * This will talloc_steal() policy_state onto the
>+		 * connection, which has longer lifetime than the
>+		 * immidiate caller requires
>+		 */
>+		status = dcesrv_iface_state_store_conn(dce_call,
>+						       LSA_SERVER_IMPLICIT_POLICY_STATE_MAGIC,
>+						       policy_state);
>+		if (!NT_STATUS_IS_OK(status)) {
>+			return status;
>+		}
> 	}
>+	*_policy_state = policy_state;
>+	return NT_STATUS_OK;
>+}
>+
>+/*
>+  lsa_LookupSids3
>+
>+  Identical to LookupSids2, but doesn't take a policy handle
>+
>+*/
>+NTSTATUS dcesrv_lsa_LookupSids3(struct dcesrv_call_state *dce_call,
>+				TALLOC_CTX *mem_ctx,
>+				struct lsa_LookupSids3 *r)
>+{
>+	struct dcesrv_lsa_LookupSids_base_state *state = NULL;
>+	NTSTATUS status;
> 
> 	*r->out.domains = NULL;
> 	r->out.names->count = 0;
>@@ -706,16 +756,27 @@ NTSTATUS dcesrv_lsa_LookupSids3(struct dcesrv_call_state *dce_call,
> 		return NT_STATUS_NO_MEMORY;
> 	}
> 
>-	state->dce_call = dce_call;
>-	state->mem_ctx = mem_ctx;
>-
>-	status = dcesrv_lsa_get_policy_state(state->dce_call, mem_ctx,
>-					     0, /* we skip access checks */
>-					     &state->policy_state);
>+	/*
>+	 * We don't have a policy handle on this call, so we want to
>+	 * make a policy state and cache it for the life of the
>+	 * connection, to avoid re-opening the DB each call.
>+	 *
>+	 * This also enforces that this is only available over
>+	 * ncacn_ip_tcp and with SCHANNEL.
>+	 *
>+	 * schannel_call_setup may also set the fault state.
>+	 *
>+	 * state->policy_state is shared between all calls on this
>+	 * connection and is moved with talloc_steal() under the
>+	 * connection, not dce_call or state.
>+	 */
>+	status = schannel_call_setup(dce_call, &state->policy_state);
> 	if (!NT_STATUS_IS_OK(status)) {
> 		return status;
> 	}
> 
>+	state->dce_call = dce_call;
>+	state->mem_ctx = mem_ctx;
> 	state->r.in.sids = r->in.sids;
> 	state->r.in.level = r->in.level;
> 	state->r.in.lookup_options = r->in.lookup_options;
>@@ -1296,25 +1357,9 @@ NTSTATUS dcesrv_lsa_LookupNames3(struct dcesrv_call_state *dce_call,
> NTSTATUS dcesrv_lsa_LookupNames4(struct dcesrv_call_state *dce_call, TALLOC_CTX *mem_ctx,
> 				 struct lsa_LookupNames4 *r)
> {
>-	enum dcerpc_transport_t transport =
>-		dcerpc_binding_get_transport(dce_call->conn->endpoint->ep_description);
>-	enum dcerpc_AuthType auth_type = DCERPC_AUTH_TYPE_NONE;
> 	struct dcesrv_lsa_LookupNames_base_state *state = NULL;
> 	NTSTATUS status;
> 
>-	if (transport != NCACN_IP_TCP) {
>-		DCESRV_FAULT(DCERPC_FAULT_ACCESS_DENIED);
>-	}
>-
>-	/*
>-	 * We don't have policy handles on this call. So this must be restricted
>-	 * to crypto connections only.
>-	 */
>-	dcesrv_call_auth_info(dce_call, &auth_type, NULL);
>-	if (auth_type != DCERPC_AUTH_TYPE_SCHANNEL) {
>-		DCESRV_FAULT(DCERPC_FAULT_ACCESS_DENIED);
>-	}
>-
> 	*r->out.domains = NULL;
> 	r->out.sids->count = 0;
> 	r->out.sids->sids = NULL;
>@@ -1328,9 +1373,21 @@ NTSTATUS dcesrv_lsa_LookupNames4(struct dcesrv_call_state *dce_call, TALLOC_CTX
> 	state->dce_call = dce_call;
> 	state->mem_ctx = mem_ctx;
> 
>-	status = dcesrv_lsa_get_policy_state(state->dce_call, state,
>-					     0, /* we skip access checks */
>-					     &state->policy_state);
>+	/*
>+	 * We don't have a policy handle on this call, so we want to
>+	 * make a policy state and cache it for the life of the
>+	 * connection, to avoid re-opening the DB each call.
>+	 *
>+	 * This also enforces that this is only available over
>+	 * ncacn_ip_tcp and with SCHANNEL.
>+	 *
>+	 * schannel_call_setup may also set the fault state.
>+	 *
>+	 * state->policy_state is shared between all calls on this
>+	 * connection and is moved with talloc_steal() under the
>+	 * connection, not dce_call or state.
>+	 */
>+	status = schannel_call_setup(dce_call, &state->policy_state);
> 	if (!NT_STATUS_IS_OK(status)) {
> 		return status;
> 	}
>-- 
>2.25.1
>
>
>From 0037c931bdd40ccddde2428e5be7edcb0febaebd Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Mon, 6 Sep 2021 08:52:21 +1200
>Subject: [PATCH 6/6] selftest: Add prefix to new schema attributes to avoid
> flapping dsdb_schema_attributes
>
>If two of these unit tests run in the same second they could
>select the same name, as the name was only based on the time
>and a common prefix.
>
>As observed by Jeremy Allison.  Thanks for the report!
>
>RN: Address flapping dsdb_schema_attributes test
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14819
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>
>Autobuild-User(master): Jeremy Allison <jra@samba.org>
>Autobuild-Date(master): Mon Sep  6 02:32:51 UTC 2021 on sn-devel-184
>
>(cherry picked from commit 6590bb0b77c641f0d4686b39c713c1405ffb64f5)
>---
> python/samba/tests/dsdb_schema_attributes.py | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/python/samba/tests/dsdb_schema_attributes.py b/python/samba/tests/dsdb_schema_attributes.py
>index bb4603d4703..d9b538d4af4 100644
>--- a/python/samba/tests/dsdb_schema_attributes.py
>+++ b/python/samba/tests/dsdb_schema_attributes.py
>@@ -87,7 +87,7 @@ systemOnly: FALSE
> 
>     def test_AddIndexedAttribute(self):
>         # create names for an attribute to add
>-        (attr_name, attr_ldap_name, attr_dn) = self._make_obj_names("schemaAttributes-Attr-")
>+        (attr_name, attr_ldap_name, attr_dn) = self._make_obj_names("schemaAttributes-IdxAttr-")
>         ldif = self._make_attr_ldif(attr_name, attr_dn, 1,
>                                     "searchFlags: %d" % samba.dsdb.SEARCH_FLAG_ATTINDEX)
> 
>@@ -111,7 +111,7 @@ systemOnly: FALSE
> 
>     def test_AddUnIndexedAttribute(self):
>         # create names for an attribute to add
>-        (attr_name, attr_ldap_name, attr_dn) = self._make_obj_names("schemaAttributes-Attr-")
>+        (attr_name, attr_ldap_name, attr_dn) = self._make_obj_names("schemaAttributes-UnIdxAttr-")
>         ldif = self._make_attr_ldif(attr_name, attr_dn, 2)
> 
>         # add the new attribute
>@@ -134,7 +134,7 @@ systemOnly: FALSE
> 
>     def test_AddTwoIndexedAttributes(self):
>         # create names for an attribute to add
>-        (attr_name, attr_ldap_name, attr_dn) = self._make_obj_names("schemaAttributes-Attr-")
>+        (attr_name, attr_ldap_name, attr_dn) = self._make_obj_names("schemaAttributes-2IdxAttr-")
>         ldif = self._make_attr_ldif(attr_name, attr_dn, 3,
>                                     "searchFlags: %d" % samba.dsdb.SEARCH_FLAG_ATTINDEX)
> 
>-- 
>2.25.1
>

Flags:

jra: review+
abartlet: ci-passed+

Actions: View

Attachments on bug 14821: 16774 | 17535