From f39a746e64daa0876ca5948784f181d39edf37a8 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Jan 2024 09:26:51 +1300 Subject: [PATCH 01/19] perftest:ndr_pack: rename SD tests with object ACEs We are looking at an optimisation for non-object ACEs, which are more common, but these tests are overwhelmed by object (OA) ACEs. Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit d5371f6bcd2fe991d08fcf2006ce62e6a7449ae9) --- source4/dsdb/tests/python/ndr_pack_performance.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/source4/dsdb/tests/python/ndr_pack_performance.py b/source4/dsdb/tests/python/ndr_pack_performance.py index 5defacf19ec9..e7586028e90a 100644 --- a/source4/dsdb/tests/python/ndr_pack_performance.py +++ b/source4/dsdb/tests/python/ndr_pack_performance.py @@ -171,27 +171,27 @@ class UserTests(samba.tests.TestCase): self.assertEqual(blob, blob2) - def test_pack_big_sd(self): + def test_pack_big_sd_with_object_aces(self): unpacked = self.get_desc(BIG_SD_SDDL) self._test_pack(unpacked) - def test_unpack_big_sd(self): + def test_unpack_big_sd_with_object_aces(self): blob = self.get_blob(BIG_SD_SDDL) self._test_unpack(blob) - def test_pack_unpack_big_sd(self): + def test_pack_unpack_big_sd_with_object_aces(self): unpacked = self.get_desc(BIG_SD_SDDL) self._test_pack_unpack(unpacked) - def test_pack_little_sd(self): + def test_pack_little_sd_with_object_aces(self): unpacked = self.get_desc(LITTLE_SD_SDDL) self._test_pack(unpacked) - def test_unpack_little_sd(self): + def test_unpack_little_sd_with_object_aces(self): blob = self.get_blob(LITTLE_SD_SDDL) self._test_unpack(blob) - def test_pack_unpack_little_sd(self): + def test_pack_unpack_little_sd_with_object_aces(self): unpacked = self.get_desc(LITTLE_SD_SDDL) self._test_pack_unpack(unpacked) -- 2.39.1 From 9c81497a47f87679446bda75a3286f9f36db8073 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Mon, 1 Jan 2024 21:48:15 +1300 Subject: [PATCH 02/19] perftest: ndr_pack_performance gets more SD types Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit e802611743a9b899c18d6eeaa0a46323b676c296) --- .../dsdb/tests/python/ndr_pack_performance.py | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/source4/dsdb/tests/python/ndr_pack_performance.py b/source4/dsdb/tests/python/ndr_pack_performance.py index e7586028e90a..0ff03a27f21b 100644 --- a/source4/dsdb/tests/python/ndr_pack_performance.py +++ b/source4/dsdb/tests/python/ndr_pack_performance.py @@ -130,6 +130,23 @@ IOID;RP;5f202010-79a5-11d0-9020-00c04fc2d4cf;bf967aba-0de6-11d0-a285-00aa0030 0aa003049e2;RU)(OA;CIIOID;RP;b7c69e6d-2cc7-11d2-854e-00a0c983f608;bf967a86-0d e6-11d0-a285-00aa003049e2;ED)""".split()) + +CONDITIONAL_ACE_SDDL = ('O:SYG:SYD:(XA;OICI;CR;;;WD;' + '(@USER.ad://ext/AuthenticationSilo == "siloname"))') + +NON_OBJECT_SDDL = ( + "O:S-1-5-21-2212615479-2695158682-2101375468-512" + "G:S-1-5-21-2212615479-2695158682-2101375468-513" + "D:P(A;OICI;FA;;;S-1-5-21-2212615479-2695158682-2101375468-512)" + "(A;OICI;FA;;;S-1-5-21-2212615479-2695158682-2101375468-519)" + "(A;OICIIO;FA;;;CO)" + "(A;OICI;FA;;;S-1-5-21-2212615479-2695158682-2101375468-512)" + "(A;OICI;FA;;;SY)" + "(A;OICI;0x1200a9;;;AU)" + "(A;OICI;0x1200a9;;;ED)") + + + # set SCALE = 100 for normal test, or 1 for testing the test. SCALE = 100 @@ -195,6 +212,30 @@ class UserTests(samba.tests.TestCase): unpacked = self.get_desc(LITTLE_SD_SDDL) self._test_pack_unpack(unpacked) + def test_pack_conditional_ace_sd(self): + unpacked = self.get_desc(CONDITIONAL_ACE_SDDL) + self._test_pack(unpacked) + + def test_unpack_conditional_ace_sd(self): + blob = self.get_blob(CONDITIONAL_ACE_SDDL) + self._test_unpack(blob) + + def test_pack_unpack_conditional_ace_sd(self): + unpacked = self.get_desc(CONDITIONAL_ACE_SDDL) + self._test_pack_unpack(unpacked) + + def test_pack_non_object_sd(self): + unpacked = self.get_desc(NON_OBJECT_SDDL) + self._test_pack(unpacked) + + def test_unpack_non_object_sd(self): + blob = self.get_blob(NON_OBJECT_SDDL) + self._test_unpack(blob) + + def test_pack_unpack_non_object_sd(self): + unpacked = self.get_desc(NON_OBJECT_SDDL) + self._test_pack_unpack(unpacked) + def test_unpack_repl_sample(self): blob = self.get_file_blob('testdata/replication-ndrpack-example.gz') self._test_unpack(blob, cycles=20, cls=drsuapi.DsGetNCChangesCtr6) -- 2.39.1 From e2e6225fb337d842060343895152ec7016fef608 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Jan 2024 09:43:01 +1300 Subject: [PATCH 03/19] perftest:ndr_pack: slightly reduce python overhead Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit d25fe2447b553087f6285c80907ca5d0debcd827) --- source4/dsdb/tests/python/ndr_pack_performance.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source4/dsdb/tests/python/ndr_pack_performance.py b/source4/dsdb/tests/python/ndr_pack_performance.py index 0ff03a27f21b..083abf6621f4 100644 --- a/source4/dsdb/tests/python/ndr_pack_performance.py +++ b/source4/dsdb/tests/python/ndr_pack_performance.py @@ -172,16 +172,16 @@ class UserTests(samba.tests.TestCase): pass def _test_pack(self, unpacked, cycles=10000): + pack = unpacked.__ndr_pack__ for i in range(SCALE * cycles): - ndr_pack(unpacked) + pack() def _test_unpack(self, blob, cycles=10000, cls=security.descriptor): for i in range(SCALE * cycles): - ndr_unpack(cls, blob) + cls().__ndr_unpack__(blob) def _test_pack_unpack(self, desc, cycles=5000, cls=security.descriptor): blob2 = ndr_pack(desc) - for i in range(SCALE * cycles): blob = ndr_pack(desc) desc = ndr_unpack(cls, blob) -- 2.39.1 From a21361618ffcb98f93ef6e0cd4a60af2c2f93b8b Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Jan 2024 01:51:56 +1300 Subject: [PATCH 04/19] perftest:ndr_pack_performance: remove irrelevant imports, options This includes removing the ANCIENT_SAMBA switch for pre-4.3, as nobody cares anymore and many tests would not run correctly anyway. Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit ceb5389260c4469a8f03ee884325ca981c18a36a) --- .../dsdb/tests/python/ndr_pack_performance.py | 55 +------------------ 1 file changed, 3 insertions(+), 52 deletions(-) diff --git a/source4/dsdb/tests/python/ndr_pack_performance.py b/source4/dsdb/tests/python/ndr_pack_performance.py index 083abf6621f4..c0c72cac99e5 100644 --- a/source4/dsdb/tests/python/ndr_pack_performance.py +++ b/source4/dsdb/tests/python/ndr_pack_performance.py @@ -4,10 +4,7 @@ import optparse import sys sys.path.insert(0, 'bin/python') -import os import samba -import samba.getopt as options -import random import gzip # We try to use the test infrastructure of Samba 4.3+, but if it @@ -16,45 +13,13 @@ import gzip # # Don't copy this horror into ordinary tests -- it is special for # performance tests that want to apply to old versions. -try: - from samba.tests.subunitrun import SubunitOptions, TestProgram - ANCIENT_SAMBA = False -except ImportError: - ANCIENT_SAMBA = True - samba.ensure_external_module("testtools", "testtools") - samba.ensure_external_module("subunit", "subunit/python") - from subunit.run import SubunitTestRunner - import unittest + +from samba.tests.subunitrun import TestProgram from samba.ndr import ndr_pack, ndr_unpack from samba.dcerpc import security from samba.dcerpc import drsuapi -parser = optparse.OptionParser("ndr_pack_performance.py [options] ") -sambaopts = options.SambaOptions(parser) -parser.add_option_group(sambaopts) -parser.add_option_group(options.VersionOptions(parser)) - -if not ANCIENT_SAMBA: - subunitopts = SubunitOptions(parser) - parser.add_option_group(subunitopts) - -# use command line creds if available -credopts = options.CredentialsOptions(parser) -parser.add_option_group(credopts) -opts, args = parser.parse_args() - -if len(args) < 1: - parser.print_usage() - sys.exit(1) - -host = args[0] - -lp = sambaopts.get_loadparm() -creds = credopts.get_credentials(lp) - -random.seed(1) - BIG_SD_SDDL = ''.join( """O:S-1-5-21-3328325300-3937145445-4190589019-512G:S-1-5-2 @@ -246,18 +211,4 @@ class UserTests(samba.tests.TestCase): self._test_pack(desc, cycles=20) -if "://" not in host: - if os.path.isfile(host): - host = "tdb://%s" % host - else: - host = "ldap://%s" % host - - -if ANCIENT_SAMBA: - runner = SubunitTestRunner() - if not runner.run(unittest.TestLoader().loadTestsFromTestCase( - UserTests)).wasSuccessful(): - sys.exit(1) - sys.exit(0) -else: - TestProgram(module=__name__, opts=subunitopts) +TestProgram(module=__name__) -- 2.39.1 From 162866e22778aa699ffa667d8089cfe7fdd0eb30 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Jan 2024 01:52:39 +1300 Subject: [PATCH 05/19] perftest:ndr_pack: use a valid dummy SID Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit 2f68545087f25e5d4c7a7742d99527c7ebbd02ab) --- source4/dsdb/tests/python/ndr_pack_performance.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/dsdb/tests/python/ndr_pack_performance.py b/source4/dsdb/tests/python/ndr_pack_performance.py index c0c72cac99e5..ed1216e6392b 100644 --- a/source4/dsdb/tests/python/ndr_pack_performance.py +++ b/source4/dsdb/tests/python/ndr_pack_performance.py @@ -126,7 +126,7 @@ class UserTests(samba.tests.TestCase): return f.read() def get_desc(self, sddl): - dummy_sid = security.dom_sid("S-2-0-0") + dummy_sid = security.dom_sid("S-1-2-3") return security.descriptor.from_sddl(sddl, dummy_sid) def get_blob(self, sddl): -- 2.39.1 From 3cb45811ee921d1f22951e6d83c14fa4e77a681e Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Jan 2024 01:54:29 +1300 Subject: [PATCH 06/19] perftest:ndr_pack: spin in do_nothing for a while The idea was to get a less jittery idea of the underlying noise, but ut is still almost instant. This I suppose is useful in indicating that this much of the test has very little overhead. Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit 93e6ea4cff2cb6bd084db27139addeea06945ea5) --- source4/dsdb/tests/python/ndr_pack_performance.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/source4/dsdb/tests/python/ndr_pack_performance.py b/source4/dsdb/tests/python/ndr_pack_performance.py index ed1216e6392b..45c1816df39b 100644 --- a/source4/dsdb/tests/python/ndr_pack_performance.py +++ b/source4/dsdb/tests/python/ndr_pack_performance.py @@ -132,9 +132,10 @@ class UserTests(samba.tests.TestCase): def get_blob(self, sddl): return ndr_pack(self.get_desc(sddl)) - def test_00_00_do_nothing(self): + def test_00_00_do_nothing(self, cycles=10000): # this gives us an idea of the overhead - pass + for i in range(SCALE * cycles): + pass def _test_pack(self, unpacked, cycles=10000): pack = unpacked.__ndr_pack__ -- 2.39.1 From 77ea1762e7956954e93c9e1ba8215bf3d9f17d96 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 5 Jan 2024 13:19:39 +1300 Subject: [PATCH 07/19] perftest: ndr_pack runs in none environment This is worth changing, because having a server running in the background can only add noise to the results. Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit 5fa663766548eac2cc5932ae03d03b79ad1751b5) --- selftest/perf_tests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/selftest/perf_tests.py b/selftest/perf_tests.py index 2aed9deded93..cfbbf0e056e2 100644 --- a/selftest/perf_tests.py +++ b/selftest/perf_tests.py @@ -26,8 +26,8 @@ plantestsuite_loadlist("samba4.ldap.ad_dc_performance.python(ad_dc_ntvfs)", '--workgroup=$DOMAIN', '$LOADLIST', '$LISTOPT']) -plantestsuite_loadlist("samba4.ndr_pack_performance.python(ad_dc_ntvfs)", - "ad_dc_ntvfs", +plantestsuite_loadlist("samba4.ndr_pack_performance.python", + "none", [python, os.path.join(samba4srcdir, "dsdb/tests/python/ndr_pack_performance.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', -- 2.39.1 From 738f164569da9b506c5bf67d7213e02b3c7004fb Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 28 Dec 2023 23:07:56 +1300 Subject: [PATCH 08/19] pidl: calculate subcontext_size only once per pull For security_ace_coda in security.idl, the sub-context size is involves a slightly non-trivial function call which returns a constant value. In all other cases, a constant expression is used, and this makes no difference. Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit 9811762775b28e16035afb2c319b55c4bf3699d3) --- pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm b/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm index 859da0a914fe..d7386d5b2cce 100644 --- a/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm +++ b/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm @@ -603,7 +603,8 @@ sub ParseSubcontextPullStart($$$$$) $self->pidl("{"); $self->indent; $self->pidl("struct ndr_pull *$subndr;"); - $self->pidl("NDR_CHECK(ndr_pull_subcontext_start($ndr, &$subndr, $l->{HEADER_SIZE}, $subcontext_size));"); + $self->pidl("ssize_t sub_size = $subcontext_size;"); + $self->pidl("NDR_CHECK(ndr_pull_subcontext_start($ndr, &$subndr, $l->{HEADER_SIZE}, sub_size));"); if (defined $l->{COMPRESSION}) { $subndr = $self->ParseCompressionPullStart($e, $l, $subndr, $env); @@ -622,7 +623,7 @@ sub ParseSubcontextPullEnd($$$$$) $self->ParseCompressionPullEnd($e, $l, $subndr, $env); } - $self->pidl("NDR_CHECK(ndr_pull_subcontext_end($ndr, $subndr, $l->{HEADER_SIZE}, $subcontext_size));"); + $self->pidl("NDR_CHECK(ndr_pull_subcontext_end($ndr, $subndr, $l->{HEADER_SIZE}, sub_size));"); $self->deindent; $self->pidl("}"); } -- 2.39.1 From 2b551fd84f8af22df8628dce4f229851775cb43b Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 31 Dec 2023 13:03:32 +1300 Subject: [PATCH 09/19] ndr: shift ndr_pull_security_ace to manual code This was manual until commit c73034cf7c4392f5d3505319948bc84634c20fa5 (a few months ago). Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit 1e6a876c2cc4b3b54895dde879492e756bb9b963) --- librpc/idl/security.idl | 2 +- librpc/ndr/ndr_sec_helper.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/librpc/idl/security.idl b/librpc/idl/security.idl index d1552475b638..b0237d8b856a 100644 --- a/librpc/idl/security.idl +++ b/librpc/idl/security.idl @@ -715,7 +715,7 @@ interface security [default][flag(NDR_REMAINING)] DATA_BLOB ignored; } security_ace_coda; - typedef [public,gensize,nosize] struct { + typedef [public,gensize,nosize,nopull] struct { security_ace_type type; /* SEC_ACE_TYPE_* */ security_ace_flags flags; /* SEC_ACE_FLAG_* */ [value(ndr_size_security_ace(r,ndr->flags))] uint16 size; diff --git a/librpc/ndr/ndr_sec_helper.c b/librpc/ndr/ndr_sec_helper.c index d74d31b28232..a634dfd75fc7 100644 --- a/librpc/ndr/ndr_sec_helper.c +++ b/librpc/ndr/ndr_sec_helper.c @@ -75,6 +75,36 @@ size_t ndr_size_security_ace(const struct security_ace *ace, libndr_flags flags) return ret; } +_PUBLIC_ enum ndr_err_code ndr_pull_security_ace(struct ndr_pull *ndr, ndr_flags_type ndr_flags, struct security_ace *r) +{ + NDR_PULL_CHECK_FLAGS(ndr, ndr_flags); + if (ndr_flags & NDR_SCALARS) { + NDR_CHECK(ndr_pull_align(ndr, 5)); + NDR_CHECK(ndr_pull_security_ace_type(ndr, NDR_SCALARS, &r->type)); + NDR_CHECK(ndr_pull_security_ace_flags(ndr, NDR_SCALARS, &r->flags)); + NDR_CHECK(ndr_pull_uint16(ndr, NDR_SCALARS, &r->size)); + NDR_CHECK(ndr_pull_uint32(ndr, NDR_SCALARS, &r->access_mask)); + NDR_CHECK(ndr_pull_set_switch_value(ndr, &r->object, sec_ace_object(r->type))); + NDR_CHECK(ndr_pull_security_ace_object_ctr(ndr, NDR_SCALARS, &r->object)); + NDR_CHECK(ndr_pull_dom_sid(ndr, NDR_SCALARS, &r->trustee)); + { + struct ndr_pull *_ndr_coda; + ssize_t sub_size = ndr_subcontext_size_of_ace_coda(r, r->size, ndr->flags); + NDR_CHECK(ndr_pull_subcontext_start(ndr, &_ndr_coda, 0, sub_size)); + NDR_CHECK(ndr_pull_set_switch_value(_ndr_coda, &r->coda, r->type)); + NDR_CHECK(ndr_pull_security_ace_coda(_ndr_coda, NDR_SCALARS|NDR_BUFFERS, &r->coda)); + NDR_CHECK(ndr_pull_subcontext_end(ndr, _ndr_coda, 0, sub_size)); + } + NDR_CHECK(ndr_pull_trailer_align(ndr, 5)); + } + if (ndr_flags & NDR_BUFFERS) { + NDR_CHECK(ndr_pull_set_switch_value(ndr, &r->object, sec_ace_object(r->type))); + NDR_CHECK(ndr_pull_security_ace_object_ctr(ndr, NDR_BUFFERS, &r->object)); + } + return NDR_ERR_SUCCESS; +} + + /* * An ACE coda can't be bigger than the space allowed for by * ace->size, so we need to check this from the context of the ACE. -- 2.39.1 From 6272e2e097e05d120194894937225bdbe3e31252 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 31 Dec 2023 13:06:40 +1300 Subject: [PATCH 10/19] ndr: short-circuit ace coda if no bytes left The overwhelmingly common case is that there are no bytes left, and regardless of the ACE type we want to store an empty blob. We know the blob will be empty if there are no bytes, so we don't need to allocate a sub-ndr and tokens list and so forth. This can save almost half the time of a security descriptor pull. Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit ac0c8ee01ea624e9c486251da2132710c2a43ddc) --- librpc/ndr/ndr_sec_helper.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/librpc/ndr/ndr_sec_helper.c b/librpc/ndr/ndr_sec_helper.c index a634dfd75fc7..6ae177f292d5 100644 --- a/librpc/ndr/ndr_sec_helper.c +++ b/librpc/ndr/ndr_sec_helper.c @@ -79,6 +79,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_security_ace(struct ndr_pull *ndr, ndr_flags { NDR_PULL_CHECK_FLAGS(ndr, ndr_flags); if (ndr_flags & NDR_SCALARS) { + ssize_t sub_size; NDR_CHECK(ndr_pull_align(ndr, 5)); NDR_CHECK(ndr_pull_security_ace_type(ndr, NDR_SCALARS, &r->type)); NDR_CHECK(ndr_pull_security_ace_flags(ndr, NDR_SCALARS, &r->flags)); @@ -87,9 +88,12 @@ _PUBLIC_ enum ndr_err_code ndr_pull_security_ace(struct ndr_pull *ndr, ndr_flags NDR_CHECK(ndr_pull_set_switch_value(ndr, &r->object, sec_ace_object(r->type))); NDR_CHECK(ndr_pull_security_ace_object_ctr(ndr, NDR_SCALARS, &r->object)); NDR_CHECK(ndr_pull_dom_sid(ndr, NDR_SCALARS, &r->trustee)); - { + sub_size = ndr_subcontext_size_of_ace_coda(r, r->size, ndr->flags); + if (sub_size == 0) { + r->coda.ignored.data = NULL; + r->coda.ignored.length = 0; + } else { struct ndr_pull *_ndr_coda; - ssize_t sub_size = ndr_subcontext_size_of_ace_coda(r, r->size, ndr->flags); NDR_CHECK(ndr_pull_subcontext_start(ndr, &_ndr_coda, 0, sub_size)); NDR_CHECK(ndr_pull_set_switch_value(_ndr_coda, &r->coda, r->type)); NDR_CHECK(ndr_pull_security_ace_coda(_ndr_coda, NDR_SCALARS|NDR_BUFFERS, &r->coda)); -- 2.39.1 From 612732c1d38461275ad2c1f07c438588cd5140b3 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 31 Dec 2023 17:30:47 +1300 Subject: [PATCH 11/19] ndr: make security_ace push manual This will allow some optimisations; in this commit we just copy the code. Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit dc08e7924c2e359afeb4b86f306868cad00189a0) --- librpc/idl/security.idl | 2 +- librpc/ndr/ndr_sec_helper.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/librpc/idl/security.idl b/librpc/idl/security.idl index b0237d8b856a..8783b6781576 100644 --- a/librpc/idl/security.idl +++ b/librpc/idl/security.idl @@ -715,7 +715,7 @@ interface security [default][flag(NDR_REMAINING)] DATA_BLOB ignored; } security_ace_coda; - typedef [public,gensize,nosize,nopull] struct { + typedef [public,gensize,nosize,nopush,nopull] struct { security_ace_type type; /* SEC_ACE_TYPE_* */ security_ace_flags flags; /* SEC_ACE_FLAG_* */ [value(ndr_size_security_ace(r,ndr->flags))] uint16 size; diff --git a/librpc/ndr/ndr_sec_helper.c b/librpc/ndr/ndr_sec_helper.c index 6ae177f292d5..b3de2f65f23a 100644 --- a/librpc/ndr/ndr_sec_helper.c +++ b/librpc/ndr/ndr_sec_helper.c @@ -108,6 +108,34 @@ _PUBLIC_ enum ndr_err_code ndr_pull_security_ace(struct ndr_pull *ndr, ndr_flags return NDR_ERR_SUCCESS; } +_PUBLIC_ enum ndr_err_code ndr_push_security_ace(struct ndr_push *ndr, ndr_flags_type ndr_flags, const struct security_ace *r) +{ + NDR_PUSH_CHECK_FLAGS(ndr, ndr_flags); + if (ndr_flags & NDR_SCALARS) { + NDR_CHECK(ndr_push_align(ndr, 5)); + NDR_CHECK(ndr_push_security_ace_type(ndr, NDR_SCALARS, r->type)); + NDR_CHECK(ndr_push_security_ace_flags(ndr, NDR_SCALARS, r->flags)); + NDR_CHECK(ndr_push_uint16(ndr, NDR_SCALARS, ndr_size_security_ace(r, ndr->flags))); + NDR_CHECK(ndr_push_uint32(ndr, NDR_SCALARS, r->access_mask)); + NDR_CHECK(ndr_push_set_switch_value(ndr, &r->object, sec_ace_object(r->type))); + NDR_CHECK(ndr_push_security_ace_object_ctr(ndr, NDR_SCALARS, &r->object)); + NDR_CHECK(ndr_push_dom_sid(ndr, NDR_SCALARS, &r->trustee)); + { + struct ndr_push *_ndr_coda; + NDR_CHECK(ndr_push_subcontext_start(ndr, &_ndr_coda, 0, ndr_subcontext_size_of_ace_coda(r, ndr_size_security_ace(r, ndr->flags), ndr->flags))); + NDR_CHECK(ndr_push_set_switch_value(_ndr_coda, &r->coda, r->type)); + NDR_CHECK(ndr_push_security_ace_coda(_ndr_coda, NDR_SCALARS|NDR_BUFFERS, &r->coda)); + NDR_CHECK(ndr_push_subcontext_end(ndr, _ndr_coda, 0, ndr_subcontext_size_of_ace_coda(r, ndr_size_security_ace(r, ndr->flags), ndr->flags))); + } + NDR_CHECK(ndr_push_trailer_align(ndr, 5)); + } + if (ndr_flags & NDR_BUFFERS) { + NDR_CHECK(ndr_push_set_switch_value(ndr, &r->object, sec_ace_object(r->type))); + NDR_CHECK(ndr_push_security_ace_object_ctr(ndr, NDR_BUFFERS, &r->object)); + } + return NDR_ERR_SUCCESS; +} + /* * An ACE coda can't be bigger than the space allowed for by -- 2.39.1 From 47db8a4676031c2c98370d83c4231c15ca3e67f3 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 31 Dec 2023 17:39:23 +1300 Subject: [PATCH 12/19] ndr: ACE push avoids no-op coda pushes We don't expect an ordinary ACE to have a non-empty coda, and we don't really want to push it if it does, but for this patch we still will. This will not change the data on the wire. Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit ee1b8ae04b10306c059174a5b4b637b080fe23fd) --- librpc/ndr/ndr_sec_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/librpc/ndr/ndr_sec_helper.c b/librpc/ndr/ndr_sec_helper.c index b3de2f65f23a..ae4006b26632 100644 --- a/librpc/ndr/ndr_sec_helper.c +++ b/librpc/ndr/ndr_sec_helper.c @@ -120,7 +120,7 @@ _PUBLIC_ enum ndr_err_code ndr_push_security_ace(struct ndr_push *ndr, ndr_flags NDR_CHECK(ndr_push_set_switch_value(ndr, &r->object, sec_ace_object(r->type))); NDR_CHECK(ndr_push_security_ace_object_ctr(ndr, NDR_SCALARS, &r->object)); NDR_CHECK(ndr_push_dom_sid(ndr, NDR_SCALARS, &r->trustee)); - { + if (sec_ace_has_extra_blob(r->type) || r->coda.ignored.length != 0) { struct ndr_push *_ndr_coda; NDR_CHECK(ndr_push_subcontext_start(ndr, &_ndr_coda, 0, ndr_subcontext_size_of_ace_coda(r, ndr_size_security_ace(r, ndr->flags), ndr->flags))); NDR_CHECK(ndr_push_set_switch_value(_ndr_coda, &r->coda, r->type)); -- 2.39.1 From 82edb4599590c196d13cf0c0e45c7525a1a00e99 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 29 Dec 2023 15:15:48 +1300 Subject: [PATCH 13/19] ndr: skip talloc when pulling empty DATA_BLOB Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit c2673b02a7a51761e8b6631eb0c0e7062cbbed7b) --- librpc/ndr/ndr_basic.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/librpc/ndr/ndr_basic.c b/librpc/ndr/ndr_basic.c index fc8620f28c77..5fd15730a74c 100644 --- a/librpc/ndr/ndr_basic.c +++ b/librpc/ndr/ndr_basic.c @@ -1453,6 +1453,12 @@ _PUBLIC_ enum ndr_err_code ndr_pull_DATA_BLOB(struct ndr_pull *ndr, ndr_flags_ty } else { NDR_CHECK(ndr_pull_uint3264(ndr, NDR_SCALARS, &length)); } + if (length == 0) { + /* skip the talloc for an empty blob */ + blob->data = NULL; + blob->length = 0; + return NDR_ERR_SUCCESS; + } NDR_PULL_NEED_BYTES(ndr, length); *blob = data_blob_talloc(ndr->current_mem_ctx, ndr->data+ndr->offset, length); ndr->offset += length; -- 2.39.1 From 4380a973df046eb1d4cf1bf8251365c3c0e15fe2 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 29 Dec 2023 15:27:08 +1300 Subject: [PATCH 14/19] ndr: mark invalid pull ndr_flags as unlikely This might have little effect, but sometimes we see primatives like ndr_pull_uint32() taking a few percent of the CPU time, and this is in all those functions. Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit 4face258dee93dcd01dce71fcb7448b285ff4860) --- librpc/ndr/libndr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/librpc/ndr/libndr.h b/librpc/ndr/libndr.h index 70dd01e49a67..03d1aead01a6 100644 --- a/librpc/ndr/libndr.h +++ b/librpc/ndr/libndr.h @@ -367,7 +367,7 @@ enum ndr_compression_alg { }; #define NDR_PULL_CHECK_FLAGS(ndr, ndr_flags) do { \ - if ((ndr_flags) & ~(NDR_SCALARS|NDR_BUFFERS)) { \ + if (unlikely((ndr_flags) & ~(NDR_SCALARS|NDR_BUFFERS))) { \ return ndr_pull_error(ndr, NDR_ERR_FLAGS, "Invalid pull struct ndr_flags 0x%"PRI_NDR_FLAGS_TYPE, ndr_flags); \ } \ } while (0) -- 2.39.1 From 15879329b300cd30af8e360216c5651fbf1576c8 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 31 Dec 2023 17:45:36 +1300 Subject: [PATCH 15/19] ndr: do not push ACE->coda.ignored blob From 1e80221b2340de5ef5e2a17f10511bbc2c041163 (2008) until c73034cf7c4392f5d3505319948bc84634c20fa5 (conditional ACEs, etc, 2023) we had a manual ndr_pull_security_ace() that would discard trailing bytes, which are those bytes that we now call the coda. The ACE types that we handled then are those that end up with a coda.ignored data blob. With this we effectively restore the long-standing behaviour in the event that we push and pull an ACE -- though now we discard the ignored bytes on push rather than pull. This change is not because the trailing bytes caused any problems (as far as is known), but because it is much faster to not do the push. It may be that such ACEs no longer occur. Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit 2a60ec98409b161cfeb4b51414ba61feb26c01b9) --- librpc/ndr/ndr_sec_helper.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/librpc/ndr/ndr_sec_helper.c b/librpc/ndr/ndr_sec_helper.c index ae4006b26632..98663a12ad9e 100644 --- a/librpc/ndr/ndr_sec_helper.c +++ b/librpc/ndr/ndr_sec_helper.c @@ -64,7 +64,11 @@ size_t ndr_size_security_ace(const struct security_ace *ace, libndr_flags flags) } else if (ace->type == SEC_ACE_TYPE_SYSTEM_RESOURCE_ATTRIBUTE) { ret += ndr_size_security_ace_coda(&ace->coda, ace->type, flags); } else { - ret += ace->coda.ignored.length; + /* + * Normal ACEs have a coda.ignored blob that is always or + * almost always empty. We aren't going to push it (it is + * ignored), so we don't add that length to the size. + */ } /* round up to a multiple of 4 (MS-DTYP 2.4.4.1) */ ret = (ret + 3ULL) & ~3ULL; @@ -120,7 +124,7 @@ _PUBLIC_ enum ndr_err_code ndr_push_security_ace(struct ndr_push *ndr, ndr_flags NDR_CHECK(ndr_push_set_switch_value(ndr, &r->object, sec_ace_object(r->type))); NDR_CHECK(ndr_push_security_ace_object_ctr(ndr, NDR_SCALARS, &r->object)); NDR_CHECK(ndr_push_dom_sid(ndr, NDR_SCALARS, &r->trustee)); - if (sec_ace_has_extra_blob(r->type) || r->coda.ignored.length != 0) { + if (sec_ace_has_extra_blob(r->type)) { struct ndr_push *_ndr_coda; NDR_CHECK(ndr_push_subcontext_start(ndr, &_ndr_coda, 0, ndr_subcontext_size_of_ace_coda(r, ndr_size_security_ace(r, ndr->flags), ndr->flags))); NDR_CHECK(ndr_push_set_switch_value(_ndr_coda, &r->coda, r->type)); -- 2.39.1 From a241a9b6b84341b6c7514b8892e85e3b67a1023d Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Mon, 1 Jan 2024 10:21:33 +1300 Subject: [PATCH 16/19] ndr: avoid object ACE pull overhead for non-object ACE When an ACE is not an object ACE, which is common, setting the switch value and attempting the object ACE GUID pull is just going to do nothing, and we know that ahead of time. By noticing that we can save a bit of time on a common operation. Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit fce4d51eb492a6fc807c6849cd4bd65ca7714509) --- librpc/ndr/ndr_sec_helper.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/librpc/ndr/ndr_sec_helper.c b/librpc/ndr/ndr_sec_helper.c index 98663a12ad9e..4e721e165051 100644 --- a/librpc/ndr/ndr_sec_helper.c +++ b/librpc/ndr/ndr_sec_helper.c @@ -79,6 +79,27 @@ size_t ndr_size_security_ace(const struct security_ace *ace, libndr_flags flags) return ret; } + +static inline enum ndr_err_code ndr_maybe_pull_security_ace_object_ctr(struct ndr_pull *ndr, + ndr_flags_type ndr_flags, + struct security_ace *r) +{ + /* + * If this is not an object ACE (as is usually common), + * ndr_pull_security_ace_object_ctr() will do nothing. + * + * By avoiding calling the function in that case, we avoid some + * tallocing and ndr token busywork. + */ + bool is_object = sec_ace_object(r->type); + if (is_object) { + NDR_CHECK(ndr_pull_set_switch_value(ndr, &r->object, is_object)); + NDR_CHECK(ndr_pull_security_ace_object_ctr(ndr, ndr_flags, &r->object)); + } + return NDR_ERR_SUCCESS; +} + + _PUBLIC_ enum ndr_err_code ndr_pull_security_ace(struct ndr_pull *ndr, ndr_flags_type ndr_flags, struct security_ace *r) { NDR_PULL_CHECK_FLAGS(ndr, ndr_flags); @@ -89,8 +110,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_security_ace(struct ndr_pull *ndr, ndr_flags NDR_CHECK(ndr_pull_security_ace_flags(ndr, NDR_SCALARS, &r->flags)); NDR_CHECK(ndr_pull_uint16(ndr, NDR_SCALARS, &r->size)); NDR_CHECK(ndr_pull_uint32(ndr, NDR_SCALARS, &r->access_mask)); - NDR_CHECK(ndr_pull_set_switch_value(ndr, &r->object, sec_ace_object(r->type))); - NDR_CHECK(ndr_pull_security_ace_object_ctr(ndr, NDR_SCALARS, &r->object)); + NDR_CHECK(ndr_maybe_pull_security_ace_object_ctr(ndr, NDR_SCALARS, r)); NDR_CHECK(ndr_pull_dom_sid(ndr, NDR_SCALARS, &r->trustee)); sub_size = ndr_subcontext_size_of_ace_coda(r, r->size, ndr->flags); if (sub_size == 0) { @@ -106,8 +126,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_security_ace(struct ndr_pull *ndr, ndr_flags NDR_CHECK(ndr_pull_trailer_align(ndr, 5)); } if (ndr_flags & NDR_BUFFERS) { - NDR_CHECK(ndr_pull_set_switch_value(ndr, &r->object, sec_ace_object(r->type))); - NDR_CHECK(ndr_pull_security_ace_object_ctr(ndr, NDR_BUFFERS, &r->object)); + NDR_CHECK(ndr_maybe_pull_security_ace_object_ctr(ndr, NDR_BUFFERS, r)); } return NDR_ERR_SUCCESS; } -- 2.39.1 From 7e9a7d57d745d14231c2523a169bcc14597f7854 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Mon, 1 Jan 2024 10:21:55 +1300 Subject: [PATCH 17/19] ndr: avoid object ACE push overhead for non-object ACE Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit ecb5da3e49283ca3a03dea81d22db4a081e192e4) --- librpc/ndr/ndr_sec_helper.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/librpc/ndr/ndr_sec_helper.c b/librpc/ndr/ndr_sec_helper.c index 4e721e165051..e07ee233599e 100644 --- a/librpc/ndr/ndr_sec_helper.c +++ b/librpc/ndr/ndr_sec_helper.c @@ -131,6 +131,24 @@ _PUBLIC_ enum ndr_err_code ndr_pull_security_ace(struct ndr_pull *ndr, ndr_flags return NDR_ERR_SUCCESS; } + +static inline enum ndr_err_code ndr_maybe_push_security_ace_object_ctr(struct ndr_push *ndr, + ndr_flags_type ndr_flags, + const struct security_ace *r) +{ + /* + * ndr_push_security_ace_object_ctr() does nothing (except tallocing + * and ndr_token fiddling) unless the ACE is an object ACE, which is + * usually very unlikely. + */ + bool is_object = sec_ace_object(r->type); + if (is_object) { + NDR_CHECK(ndr_push_set_switch_value(ndr, &r->object, is_object)); + NDR_CHECK(ndr_push_security_ace_object_ctr(ndr, ndr_flags, &r->object)); + } + return NDR_ERR_SUCCESS; +} + _PUBLIC_ enum ndr_err_code ndr_push_security_ace(struct ndr_push *ndr, ndr_flags_type ndr_flags, const struct security_ace *r) { NDR_PUSH_CHECK_FLAGS(ndr, ndr_flags); @@ -140,8 +158,7 @@ _PUBLIC_ enum ndr_err_code ndr_push_security_ace(struct ndr_push *ndr, ndr_flags NDR_CHECK(ndr_push_security_ace_flags(ndr, NDR_SCALARS, r->flags)); NDR_CHECK(ndr_push_uint16(ndr, NDR_SCALARS, ndr_size_security_ace(r, ndr->flags))); NDR_CHECK(ndr_push_uint32(ndr, NDR_SCALARS, r->access_mask)); - NDR_CHECK(ndr_push_set_switch_value(ndr, &r->object, sec_ace_object(r->type))); - NDR_CHECK(ndr_push_security_ace_object_ctr(ndr, NDR_SCALARS, &r->object)); + NDR_CHECK(ndr_maybe_push_security_ace_object_ctr(ndr, NDR_SCALARS, r)); NDR_CHECK(ndr_push_dom_sid(ndr, NDR_SCALARS, &r->trustee)); if (sec_ace_has_extra_blob(r->type)) { struct ndr_push *_ndr_coda; @@ -153,8 +170,7 @@ _PUBLIC_ enum ndr_err_code ndr_push_security_ace(struct ndr_push *ndr, ndr_flags NDR_CHECK(ndr_push_trailer_align(ndr, 5)); } if (ndr_flags & NDR_BUFFERS) { - NDR_CHECK(ndr_push_set_switch_value(ndr, &r->object, sec_ace_object(r->type))); - NDR_CHECK(ndr_push_security_ace_object_ctr(ndr, NDR_BUFFERS, &r->object)); + NDR_CHECK(ndr_maybe_push_security_ace_object_ctr(ndr, NDR_BUFFERS, r)); } return NDR_ERR_SUCCESS; } -- 2.39.1 From 84e28516e44ce56115752cbf590950606da83254 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Mon, 8 Jan 2024 14:50:30 +1300 Subject: [PATCH 18/19] ndr: ndr_push_security_ace: calculate coda size once Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit a72c198921f64f2502f543c7158762c64cb3074e) --- librpc/ndr/ndr_sec_helper.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/librpc/ndr/ndr_sec_helper.c b/librpc/ndr/ndr_sec_helper.c index e07ee233599e..508bcd219dda 100644 --- a/librpc/ndr/ndr_sec_helper.c +++ b/librpc/ndr/ndr_sec_helper.c @@ -162,10 +162,14 @@ _PUBLIC_ enum ndr_err_code ndr_push_security_ace(struct ndr_push *ndr, ndr_flags NDR_CHECK(ndr_push_dom_sid(ndr, NDR_SCALARS, &r->trustee)); if (sec_ace_has_extra_blob(r->type)) { struct ndr_push *_ndr_coda; - NDR_CHECK(ndr_push_subcontext_start(ndr, &_ndr_coda, 0, ndr_subcontext_size_of_ace_coda(r, ndr_size_security_ace(r, ndr->flags), ndr->flags))); + size_t coda_size = ndr_subcontext_size_of_ace_coda( + r, + ndr_size_security_ace(r, ndr->flags), + ndr->flags); + NDR_CHECK(ndr_push_subcontext_start(ndr, &_ndr_coda, 0, coda_size)); NDR_CHECK(ndr_push_set_switch_value(_ndr_coda, &r->coda, r->type)); NDR_CHECK(ndr_push_security_ace_coda(_ndr_coda, NDR_SCALARS|NDR_BUFFERS, &r->coda)); - NDR_CHECK(ndr_push_subcontext_end(ndr, _ndr_coda, 0, ndr_subcontext_size_of_ace_coda(r, ndr_size_security_ace(r, ndr->flags), ndr->flags))); + NDR_CHECK(ndr_push_subcontext_end(ndr, _ndr_coda, 0, coda_size)); } NDR_CHECK(ndr_push_trailer_align(ndr, 5)); } -- 2.39.1 From 15571366c889cc2a7f24f938f224f3c1b79b0cc7 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Mon, 8 Jan 2024 15:05:35 +1300 Subject: [PATCH 19/19] ndr: ignore trailing bytes in ndr_pull_security_ace() This returns the behaviour with ordinary ACEs to where it was with 4.19. Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=15574 (cherry picked from commit 0c1f421c107be3156b3f1db75aced24a1bca3d2f) --- librpc/ndr/ndr_sec_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/librpc/ndr/ndr_sec_helper.c b/librpc/ndr/ndr_sec_helper.c index 508bcd219dda..f870a17aafc6 100644 --- a/librpc/ndr/ndr_sec_helper.c +++ b/librpc/ndr/ndr_sec_helper.c @@ -113,7 +113,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_security_ace(struct ndr_pull *ndr, ndr_flags NDR_CHECK(ndr_maybe_pull_security_ace_object_ctr(ndr, NDR_SCALARS, r)); NDR_CHECK(ndr_pull_dom_sid(ndr, NDR_SCALARS, &r->trustee)); sub_size = ndr_subcontext_size_of_ace_coda(r, r->size, ndr->flags); - if (sub_size == 0) { + if (!sec_ace_has_extra_blob(r->type) || sub_size == 0) { r->coda.ignored.data = NULL; r->coda.ignored.length = 0; } else { -- 2.39.1