From 8ca79bbc446d9e2f3517fc368f2b828d156721b1 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 11:06:00 +1300 Subject: [PATCH 01/65] ldb: avoid out of bounds read and write in ldb_qsort() If a compare function is non-transitive (for example, if it evaluates A > B and B > C, but A < C), this implementation of qsort could access out-of-bounds memory. This was found in glibc's qsort by Qualys, and their write-up for OSS-Security explains it very well: https://www.openwall.com/lists/oss-security/2024/01/30/7 An example of a non-transitive compare is one in which does this int cmp(const void *_a, const void *_b) { int a = *(int *)_a; int b = *(int *)_b; return a - b; } which does the right thing when the magnitude of the numbers is small, but which will go wrong if a is INT_MIN and b is INT_MAX. Likewise, if a and b are e.g. uint32_t, the value can wrap when cast to int. We have functions that are non-transitive regardless of subtraction. For example, here (which is not used with ldb_qsort): int codepoint_cmpi(codepoint_t c1, codepoint_t c2) if (c1 == c2 || toupper_m(c1) == toupper_m(c2)) { return 0; } return c1 - c2; } The toupper_m() is only called on equality case. Consider {'a', 'A', 'B'}. 'a' == 'A' 'a' > 'B' (lowercase letters come after upper) 'A' < 'B' BUG: https://bugzilla.samba.org/show_bug.cgi?id=15569 BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 73e4f6026ad04b73074b413bd8c838ca48ffde7f) --- lib/ldb/common/qsort.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldb/common/qsort.c b/lib/ldb/common/qsort.c index 012aaf3c403..bae35e6b1b1 100644 --- a/lib/ldb/common/qsort.c +++ b/lib/ldb/common/qsort.c @@ -227,7 +227,7 @@ void ldb_qsort (void *const pbase, size_t total_elems, size_t size, while ((run_ptr += size) <= end_ptr) { tmp_ptr = run_ptr - size; - while ((*cmp) ((void *) run_ptr, (void *) tmp_ptr, opaque) < 0) + while (tmp_ptr > base_ptr && (*cmp) ((void *) run_ptr, (void *) tmp_ptr, opaque) < 0) tmp_ptr -= size; tmp_ptr += size; -- 2.34.1 From 0a2d34c608b871d85c5e34863d874ffdfaee71aa Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 28 Mar 2024 12:57:54 +1300 Subject: [PATCH 02/65] lib/fuzzing/decode_ndr_X_crash: guess the pipe from filename Usually we are dealing with a filename that tells you what the pipe is, and there is no reason for this debug helper not to be convenient BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 8b6a584170eeb5082a188879be88e5f414b0be81) --- lib/fuzzing/decode_ndr_X_crash | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/fuzzing/decode_ndr_X_crash b/lib/fuzzing/decode_ndr_X_crash index 63c3cd747d7..d90e7efe122 100755 --- a/lib/fuzzing/decode_ndr_X_crash +++ b/lib/fuzzing/decode_ndr_X_crash @@ -61,8 +61,9 @@ def process_one_file(f): def main(): parser = argparse.ArgumentParser() - parser.add_argument('-p', '--pipe', default='$PIPE', - help='pipe name (for output command line)') + parser.add_argument('-p', '--pipe', default=None, + help=('pipe name (for output command line, ' + 'default is a guess or "$PIPE")')) parser.add_argument('-t', '--type', default=None, choices=TYPES, help='restrict to this type') parser.add_argument('-o', '--opnum', default=None, type=int, @@ -91,6 +92,13 @@ def main(): sys.exit(1) for fn in args.FILES: + if pipe is None: + m = re.search(r'clusterfuzz-testcase.+-fuzz_ndr_([a-z]+)', fn) + if m is None: + pipe = '$PIPE' + else: + pipe = m.group(1) + if args.crash_filter is not None: if not re.search(args.crash_filter, fn): print_if_verbose(f"skipping {fn}") -- 2.34.1 From 1a6040a4e0722c0a0d872fbbff71e0f9a9b3c143 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:43:27 +1300 Subject: [PATCH 03/65] util:tsort.h: add a macro for safely comparing numbers In many places we use `return a - b;` in a comparison function. This can be problematic if the comparison is used in a sort, as `a - b` is not guaranteed to do what we expect. For example: * if a and b are 2s-complement ints, a is INT_MIN and b is INT_MAX, then a - b = 1, which is wrong. * if a and b are 64 bit pointers, a - b could wrap around many times in a cmp function returning 32 bit ints. (We do this often). The issue is not just that a sort could go haywire. Due to a bug in glibc, this could result in out-of-bounds access: https://www.openwall.com/lists/oss-security/2024/01/30/7 (We have replicated this bug in ldb_qsort). BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 5ab93f48c575db1a3c5a707258cc44f707a5eeb0) --- lib/util/tsort.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/lib/util/tsort.h b/lib/util/tsort.h index 811d6cd2f77..18e82d6c9fe 100644 --- a/lib/util/tsort.h +++ b/lib/util/tsort.h @@ -37,4 +37,23 @@ do { \ } while (0) #endif + +#ifndef NUMERIC_CMP +/* + * NUMERIC_CMP is a safe replacement for `a - b` in comparison + * functions. It will work on integers, pointers, and floats. + * + * Rather than + * + * return a - b; + * + * use + * + * return NUMERIC_CMP(a, b); + * + * and you won't have any troubles if a - b would overflow. + */ +#define NUMERIC_CMP(a, b) (((a) > (b)) - ((a) < (b))) +#endif + #endif -- 2.34.1 From 27c580b43a52f7b03a9bb0ad7365580b8ea4ee65 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 17:53:39 +1300 Subject: [PATCH 04/65] ldb: add NUMERIC_CMP macro to ldb.h In other places we tend to include tsort.h, which also has TYPESAFE_QSORT. ldb.h already has TYPESAFE_QSORT, so it might as well have NUMERIC_CMP. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit de1b94f79ea8694ecdddab4b455d539caa7e77e2) --- lib/ldb/include/ldb.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/ldb/include/ldb.h b/lib/ldb/include/ldb.h index 8bebd9fce1b..c495e7f1750 100644 --- a/lib/ldb/include/ldb.h +++ b/lib/ldb/include/ldb.h @@ -2326,6 +2326,22 @@ do { \ } while (0) #endif +#ifndef NUMERIC_CMP +/* + * NUMERIC_CMP is a safe replacement for `a - b` in comparison + * functions. It will work on integers, pointers, and floats. + * + * Rather than + * + * return a - b; + * + * use + * + * return NUMERIC_CMP(a, b); + */ +#define NUMERIC_CMP(a, b) (((a) > (b)) - ((a) < (b))) +#endif + /** -- 2.34.1 From 917751d60b1e0c43ad69b36843c31dea3ab0f7ec Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:50:47 +1300 Subject: [PATCH 05/65] ldb:ldb_dn: use safe NUMERIC_CMP in ldb_dn_compare_base() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 5150b318f4894a8036b2a394c446afd513f8cb60) --- lib/ldb/common/ldb_dn.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldb/common/ldb_dn.c b/lib/ldb/common/ldb_dn.c index 601da57a1b1..7beea6e6535 100644 --- a/lib/ldb/common/ldb_dn.c +++ b/lib/ldb/common/ldb_dn.c @@ -1111,7 +1111,7 @@ int ldb_dn_compare_base(struct ldb_dn *base, struct ldb_dn *dn) /* compare attr.cf_value. */ if (b_vlen != dn_vlen) { - return b_vlen - dn_vlen; + return NUMERIC_CMP(b_vlen, dn_vlen); } ret = strncmp(b_vdata, dn_vdata, b_vlen); if (ret != 0) return ret; -- 2.34.1 From 80c0eea958089a67bb9db4e405bc2b2797072dac Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:51:04 +1300 Subject: [PATCH 06/65] ldb:ldb_dn: use safe NUMERIC_CMP in ldb_dn_compare() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 75e51bd99b7a029afd98b55283eddad835319ed6) --- lib/ldb/common/ldb_dn.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldb/common/ldb_dn.c b/lib/ldb/common/ldb_dn.c index 7beea6e6535..7c0f0a2197b 100644 --- a/lib/ldb/common/ldb_dn.c +++ b/lib/ldb/common/ldb_dn.c @@ -1190,7 +1190,7 @@ int ldb_dn_compare(struct ldb_dn *dn0, struct ldb_dn *dn1) /* compare attr.cf_value. */ if (dn0_vlen != dn1_vlen) { - return dn0_vlen - dn1_vlen; + return NUMERIC_CMP(dn0_vlen, dn1_vlen); } ret = strncmp(dn0_vdata, dn1_vdata, dn0_vlen); if (ret != 0) { -- 2.34.1 From b47a62b1ca6a0ffecc6629d21eef8b8fe38184aa Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:52:50 +1300 Subject: [PATCH 07/65] s4:ntvfs: use NUMERIC_CMP in stream_name_cmp BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit a6d76d6ee9f7cfcabe2c20b872b8b1cb598928a6) --- source4/ntvfs/posix/pvfs_streams.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source4/ntvfs/posix/pvfs_streams.c b/source4/ntvfs/posix/pvfs_streams.c index cacd8c19952..ae1ec113e8a 100644 --- a/source4/ntvfs/posix/pvfs_streams.c +++ b/source4/ntvfs/posix/pvfs_streams.c @@ -22,6 +22,7 @@ #include "includes.h" #include "vfs_posix.h" #include "librpc/gen_ndr/xattr.h" +#include "lib/util/tsort.h" /* normalise a stream name, removing a :$DATA suffix if there is one @@ -51,7 +52,7 @@ static int stream_name_cmp(const char *name1, const char *name2) l1 = c1?(c1 - name1):strlen(name1); l2 = c2?(c2 - name2):strlen(name2); if (l1 != l2) { - return l1 - l2; + return NUMERIC_CMP(l1, l2); } ret = strncasecmp_m(name1, name2, l1); if (ret != 0) { -- 2.34.1 From 1e0450be22af6ebdf97fde858fc20ef4d9444dbd Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:55:27 +1300 Subject: [PATCH 08/65] s4:dsdb:mod:operational: use NUMERIC_CMP in pso_compare prec_{1,2} are uint32_t, and if one is not set we are defaulting to 0xffffffff (a.k.a UINT32_MAX), so an overflow when cast to int seems extremely likely. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 623adcf4aae00ac06e82d98a75ce4644890501e6) --- source4/dsdb/samdb/ldb_modules/operational.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/dsdb/samdb/ldb_modules/operational.c b/source4/dsdb/samdb/ldb_modules/operational.c index 8821765a703..8c8bfa89dde 100644 --- a/source4/dsdb/samdb/ldb_modules/operational.c +++ b/source4/dsdb/samdb/ldb_modules/operational.c @@ -1073,7 +1073,7 @@ static int pso_compare(struct ldb_message **m1, struct ldb_message **m2) return ndr_guid_compare(&guid1, &guid2); } else { - return prec1 - prec2; + return NUMERIC_CMP(prec1, prec2); } } -- 2.34.1 From a2393f614a869071a85296bf199e1e8aa07096ba Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:55:54 +1300 Subject: [PATCH 09/65] s4: use numeric_cmp in dns_common_sort_zones() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit ee4ebcccd7d9d89dda59615b3653df2632fb1a5d) --- source4/dns_server/dnsserver_common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/dns_server/dnsserver_common.c b/source4/dns_server/dnsserver_common.c index 0481b0715c7..0bbba7ff6de 100644 --- a/source4/dns_server/dnsserver_common.c +++ b/source4/dns_server/dnsserver_common.c @@ -1402,7 +1402,7 @@ static int dns_common_sort_zones(struct ldb_message **m1, struct ldb_message **m /* If the string lengths are not equal just sort by length */ if (l1 != l2) { /* If m1 is the larger zone name, return it first */ - return l2 - l1; + return NUMERIC_CMP(l2, l1); } /*TODO: We need to compare DNs here, we want the DomainDNSZones first */ -- 2.34.1 From 441a644e678f74f47c0dbde9a4ee1e803aefa35d Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 15:47:10 +1300 Subject: [PATCH 10/65] util:binsearch: user NUMERIC_CMP() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 09c98ff1263eb05933f1956e201655dd41e28a0c) --- lib/util/tests/binsearch.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/util/tests/binsearch.c b/lib/util/tests/binsearch.c index b3ecda165f3..24840156c73 100644 --- a/lib/util/tests/binsearch.c +++ b/lib/util/tests/binsearch.c @@ -23,17 +23,19 @@ #include "includes.h" #include "lib/util/binsearch.h" +#include "lib/util/tsort.h" #include "torture/torture.h" #include "torture/local/proto.h" static int int_cmp(int a, int b) { - return a - b; + return NUMERIC_CMP(a, b); } static int int_cmp_p(int a, int *b) { - return a - *b; + int _b = *b; + return NUMERIC_CMP(a, _b); } static bool test_binsearch_v(struct torture_context *tctx) -- 2.34.1 From 1e7a65f862b5440d7817d90806718ac52bf85dc1 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 5 Apr 2024 13:14:38 +1300 Subject: [PATCH 11/65] torture:charset: use < and > assertions for strcasecmp_m strcasecmp_m is supposed to return a negative, zero, or positive number, depending on whether the first argument is less than, equal to, or greater than the second argument (respectively). We have been asserting that it returns exactly the difference between the codepoints in the first character that differs. This fixes a knownfail on 32 bit. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit ac0a8cd92ca4497bfcfad30e2b4d47547b582b92) --- lib/util/charset/tests/charset.c | 14 +++++++------- selftest/knownfail-32bit | 4 ---- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/lib/util/charset/tests/charset.c b/lib/util/charset/tests/charset.c index 547dc51e59d..94bf76c010d 100644 --- a/lib/util/charset/tests/charset.c +++ b/lib/util/charset/tests/charset.c @@ -72,16 +72,16 @@ static bool test_strcasecmp_m(struct torture_context *tctx) const char file_iso8859_1[7] = { 0x66, 0x69, 0x6c, 0x65, 0x2d, 0xe9, 0 }; /* file.{accented e} in utf8 */ const char file_utf8[8] = { 0x66, 0x69, 0x6c, 0x65, 0x2d, 0xc3, 0xa9, 0 }; - torture_assert_int_equal(tctx, strcasecmp_m("foo", "bar"), 4, "different strings both lower"); - torture_assert_int_equal(tctx, strcasecmp_m("foo", "Bar"), 4, "different strings lower/upper"); - torture_assert_int_equal(tctx, strcasecmp_m("Foo", "bar"), 4, "different strings upper/lower"); - torture_assert_int_equal(tctx, strcasecmp_m("AFoo", "_bar"), 2, "different strings upper/lower"); + torture_assert_int_greater(tctx, strcasecmp_m("foo", "bar"), 0, "different strings both lower"); + torture_assert_int_greater(tctx, strcasecmp_m("foo", "Bar"), 0, "different strings lower/upper"); + torture_assert_int_greater(tctx, strcasecmp_m("Foo", "bar"), 0, "different strings upper/lower"); + torture_assert_int_greater(tctx, strcasecmp_m("AFoo", "_bar"), 0, "different strings upper/lower"); torture_assert_int_equal(tctx, strcasecmp_m("foo", "foo"), 0, "same case strings"); torture_assert_int_equal(tctx, strcasecmp_m("foo", "Foo"), 0, "different case strings"); - torture_assert_int_equal(tctx, strcasecmp_m(NULL, "Foo"), -1, "one NULL"); - torture_assert_int_equal(tctx, strcasecmp_m("foo", NULL), 1, "other NULL"); + torture_assert_int_less(tctx, strcasecmp_m(NULL, "Foo"), 0, "one NULL"); + torture_assert_int_greater(tctx, strcasecmp_m("foo", NULL), 0, "other NULL"); torture_assert_int_equal(tctx, strcasecmp_m(NULL, NULL), 0, "both NULL"); - torture_assert_int_equal(tctx, strcasecmp_m(file_iso8859_1, file_utf8), 38, + torture_assert_int_greater(tctx, strcasecmp_m(file_iso8859_1, file_utf8), 0, "file.{accented e} should differ"); return true; } diff --git a/selftest/knownfail-32bit b/selftest/knownfail-32bit index 2946f3e9936..5cb896f14fe 100644 --- a/selftest/knownfail-32bit +++ b/selftest/knownfail-32bit @@ -65,9 +65,6 @@ # [171(1386)/261 at 6m24s, 4 errors] samba4.local.charset # UNEXPECTED(failure): samba4.local.charset.strcasecmp(none) # REASON: Exception: Exception: ../../lib/util/charset/tests/charset.c:56: strcasecmp("foo", "bar") was 1 (0x1), expected 4 (0x4): different strings both lower -# UNEXPECTED(failure): samba4.local.charset.strcasecmp_m(none) -# REASON: Exception: Exception: ../../lib/util/charset/tests/charset.c:85: strcasecmp_m(file_iso8859_1, file_utf8) was 1 (0x1), expected 38 (0x26): file.{accented e} -# should differ # UNEXPECTED(failure): samba4.local.charset.strncasecmp(none) # REASON: Exception: Exception: ../../lib/util/charset/tests/charset.c:132: strncasecmp("foo", "bar", 3) was 1 (0x1), expected 4 (0x4): different strings both lower # UNEXPECTED(failure): samba4.local.charset.strncasecmp_m(none) @@ -82,7 +79,6 @@ # ERROR: Testsuite[samba4.local.charset] # REASON: Exit code was 1 ^samba4.local.charset.strcasecmp.none -^samba4.local.charset.strcasecmp_m.none ^samba4.local.charset.strncasecmp.none ^samba4.local.charset.strncasecmp_m.none # -- 2.34.1 From 58cc6a435e0395b6a08b7e50625a20a5879d422d Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 5 Apr 2024 14:43:42 +1300 Subject: [PATCH 12/65] torture:charset: use < and > assertions for strncasecmp_m strncasecmp_m is supposed to return a negative, zero, or positive number, not necessarily the difference between the codepoints in the first character that differs, which we have been asserting up to now. This fixes a knownfail on 32 bit. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit dda0bb6fc71bae91f3158f69462cb79fdad210fb) --- lib/util/charset/tests/charset.c | 14 +++++++------- selftest/knownfail-32bit | 4 ---- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/lib/util/charset/tests/charset.c b/lib/util/charset/tests/charset.c index 94bf76c010d..6fca3f36c19 100644 --- a/lib/util/charset/tests/charset.c +++ b/lib/util/charset/tests/charset.c @@ -151,19 +151,19 @@ static bool test_strncasecmp_m(struct torture_context *tctx) const char file_iso8859_1[7] = { 0x66, 0x69, 0x6c, 0x65, 0x2d, 0xe9, 0 }; /* file.{accented e} in utf8 */ const char file_utf8[8] = { 0x66, 0x69, 0x6c, 0x65, 0x2d, 0xc3, 0xa9, 0 }; - torture_assert_int_equal(tctx, strncasecmp_m("foo", "bar", 3), 4, "different strings both lower"); - torture_assert_int_equal(tctx, strncasecmp_m("foo", "Bar", 3), 4, "different strings lower/upper"); - torture_assert_int_equal(tctx, strncasecmp_m("Foo", "bar", 3), 4, "different strings upper/lower"); - torture_assert_int_equal(tctx, strncasecmp_m("AFoo", "_bar", 4), 2, "different strings upper/lower"); + torture_assert_int_greater(tctx, strncasecmp_m("foo", "bar", 3), 0, "different strings both lower"); + torture_assert_int_greater(tctx, strncasecmp_m("foo", "Bar", 3), 0, "different strings lower/upper"); + torture_assert_int_greater(tctx, strncasecmp_m("Foo", "bar", 3), 0, "different strings upper/lower"); + torture_assert_int_greater(tctx, strncasecmp_m("AFoo", "_bar", 4), 0, "different strings upper/lower"); torture_assert_int_equal(tctx, strncasecmp_m("foo", "foo", 3), 0, "same case strings"); torture_assert_int_equal(tctx, strncasecmp_m("foo", "Foo", 3), 0, "different case strings"); torture_assert_int_equal(tctx, strncasecmp_m("fool", "Foo", 3),0, "different case strings"); torture_assert_int_equal(tctx, strncasecmp_m("fool", "Fool", 40), 0, "over size"); torture_assert_int_equal(tctx, strncasecmp_m("BLA", "Fool", 0),0, "empty"); - torture_assert_int_equal(tctx, strncasecmp_m(NULL, "Foo", 3), -1, "one NULL"); - torture_assert_int_equal(tctx, strncasecmp_m("foo", NULL, 3), 1, "other NULL"); + torture_assert_int_less(tctx, strncasecmp_m(NULL, "Foo", 3), 0, "one NULL"); + torture_assert_int_greater(tctx, strncasecmp_m("foo", NULL, 3), 0, "other NULL"); torture_assert_int_equal(tctx, strncasecmp_m(NULL, NULL, 3), 0, "both NULL"); - torture_assert_int_equal(tctx, strncasecmp_m(file_iso8859_1, file_utf8, 6), 38, + torture_assert_int_greater(tctx, strncasecmp_m(file_iso8859_1, file_utf8, 6), 0, "file.{accented e} should differ"); return true; } diff --git a/selftest/knownfail-32bit b/selftest/knownfail-32bit index 5cb896f14fe..8ab625d969e 100644 --- a/selftest/knownfail-32bit +++ b/selftest/knownfail-32bit @@ -67,9 +67,6 @@ # REASON: Exception: Exception: ../../lib/util/charset/tests/charset.c:56: strcasecmp("foo", "bar") was 1 (0x1), expected 4 (0x4): different strings both lower # UNEXPECTED(failure): samba4.local.charset.strncasecmp(none) # REASON: Exception: Exception: ../../lib/util/charset/tests/charset.c:132: strncasecmp("foo", "bar", 3) was 1 (0x1), expected 4 (0x4): different strings both lower -# UNEXPECTED(failure): samba4.local.charset.strncasecmp_m(none) -# REASON: Exception: Exception: ../../lib/util/charset/tests/charset.c:167: strncasecmp_m(file_iso8859_1, file_utf8, 6) was 1 (0x1), expected 38 (0x26): file.{accent -# ed e} should differ # command: /home/samba/samba.git/bin/smbtorture $LOADLIST --configfile=$SMB_CONF_PATH --option='fss:sequence timeout=1' --maximum-runtime=$SELFTEST_MAXTIME --based # ir=$SELFTEST_TMPDIR --format=subunit --option=torture:progress=no --target=samba4 ncalrpc:localhost local.charset 2>&1 | python3 /home/samba/samba.git/selftest/fi # lter-subunit --fail-on-empty --prefix="samba4.local.charset." --suffix="(none)" @@ -80,7 +77,6 @@ # REASON: Exit code was 1 ^samba4.local.charset.strcasecmp.none ^samba4.local.charset.strncasecmp.none -^samba4.local.charset.strncasecmp_m.none # # [229(2702)/261 at 8m44s, 5 errors] samba.tests.samba_tool.provision_lmdb_size # UNEXPECTED(failure): samba.tests.samba_tool.provision_lmdb_size.samba.tests.samba_tool.provision_lmdb_size.ProvisionLmdbSizeTestCase.test_134217728b(none) -- 2.34.1 From 6c63a079c223ea3a86f9ab6433a4d55ce35b31d7 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 5 Apr 2024 14:46:48 +1300 Subject: [PATCH 13/65] torture:charset: test more of strcasecmp_m We now test cases: 1. where the first string compares less 2. one of the strings ends before the other 3. the strings differ on a character other than the first. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit a512759d7b216cacc0a780b3304549b7945f919c) --- lib/util/charset/tests/charset.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/util/charset/tests/charset.c b/lib/util/charset/tests/charset.c index 6fca3f36c19..bca5449c579 100644 --- a/lib/util/charset/tests/charset.c +++ b/lib/util/charset/tests/charset.c @@ -73,11 +73,14 @@ static bool test_strcasecmp_m(struct torture_context *tctx) /* file.{accented e} in utf8 */ const char file_utf8[8] = { 0x66, 0x69, 0x6c, 0x65, 0x2d, 0xc3, 0xa9, 0 }; torture_assert_int_greater(tctx, strcasecmp_m("foo", "bar"), 0, "different strings both lower"); + torture_assert_int_less(tctx, strcasecmp_m("bar", "foo"), 0, "different strings both lower"); torture_assert_int_greater(tctx, strcasecmp_m("foo", "Bar"), 0, "different strings lower/upper"); torture_assert_int_greater(tctx, strcasecmp_m("Foo", "bar"), 0, "different strings upper/lower"); torture_assert_int_greater(tctx, strcasecmp_m("AFoo", "_bar"), 0, "different strings upper/lower"); torture_assert_int_equal(tctx, strcasecmp_m("foo", "foo"), 0, "same case strings"); torture_assert_int_equal(tctx, strcasecmp_m("foo", "Foo"), 0, "different case strings"); + torture_assert_int_greater(tctx, strcasecmp_m("food", "Foo"), 0, "strings differ towards the end"); + torture_assert_int_less(tctx, strcasecmp_m("food", "Fool"), 0, "strings differ towards the end"); torture_assert_int_less(tctx, strcasecmp_m(NULL, "Foo"), 0, "one NULL"); torture_assert_int_greater(tctx, strcasecmp_m("foo", NULL), 0, "other NULL"); torture_assert_int_equal(tctx, strcasecmp_m(NULL, NULL), 0, "both NULL"); -- 2.34.1 From 97e98632e12cf9cb1f005592fcf2f08686b0e7fe Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 15:49:55 +1300 Subject: [PATCH 14/65] util:charset:util_str: use NUMERIC_CMP in strcasecmp_m_handle BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit f788a399996a73b2aa206ec2b15f5943b06660e0) --- lib/util/charset/util_str.c | 5 +++-- selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard diff --git a/lib/util/charset/util_str.c b/lib/util/charset/util_str.c index 5d415f097e4..04d69dfb428 100644 --- a/lib/util/charset/util_str.c +++ b/lib/util/charset/util_str.c @@ -26,6 +26,7 @@ #include "system/locale.h" #include "charset.h" #include "lib/util/fault.h" +#include "lib/util/tsort.h" #ifdef strcasecmp #undef strcasecmp @@ -79,10 +80,10 @@ _PUBLIC_ int strcasecmp_m_handle(struct smb_iconv_handle *iconv_handle, continue; } - return l1 - l2; + return NUMERIC_CMP(l1, l2); } - return *s1 - *s2; + return NUMERIC_CMP(*s1, *s2); } /** diff --git a/selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard b/selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard new file mode 100644 index 00000000000..fe0d14e83e2 --- /dev/null +++ b/selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard @@ -0,0 +1 @@ +^samba.unittests.ms_fnmatch.test_ms_fn_match_protocol_no_wildcard -- 2.34.1 From 99274172795892ac43f246514aff72d6ceaf0d9a Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 17:23:15 +1300 Subject: [PATCH 15/65] util:test: test_ms_fn_match_protocol_no_wildcard: allow -1 We have changed strcasecmp_m() to return -1 in a place where it used to return -3. This upset a test, but it shouldn't have: the exact value of the negative int is not guaranteed by the function. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit d4ce8231f986a359dc657cd1a6b416270a53c7d3) --- lib/util/tests/test_ms_fnmatch.c | 2 +- selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) delete mode 100644 selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard diff --git a/lib/util/tests/test_ms_fnmatch.c b/lib/util/tests/test_ms_fnmatch.c index d11c7bed4be..2261f9bb111 100644 --- a/lib/util/tests/test_ms_fnmatch.c +++ b/lib/util/tests/test_ms_fnmatch.c @@ -36,7 +36,7 @@ static void test_ms_fn_match_protocol_no_wildcard(void **state) /* no wildcards in pattern, a simple strcasecmp_m */ cmp = ms_fnmatch_protocol("pattern", "string", PROTOCOL_COREPLUS, true); /* case sensitive */ - assert_int_equal(cmp, -3); + assert_true(cmp < 0); } static void test_ms_fn_match_protocol_pattern_upgraded(void **state) diff --git a/selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard b/selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard deleted file mode 100644 index fe0d14e83e2..00000000000 --- a/selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard +++ /dev/null @@ -1 +0,0 @@ -^samba.unittests.ms_fnmatch.test_ms_fn_match_protocol_no_wildcard -- 2.34.1 From 4339776c79f7d32e5798238af7c849b1146abebe Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 15:53:29 +1300 Subject: [PATCH 16/65] util:charset:codepoints: condepoint_cmpi uses NUMERIC_CMP() If these are truly unicode codepoints (< ~2m) there is no overflow, but the type is defined as uint32_t. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 675fdeee3d6570fdf5a055890dc3386a8db5fd88) --- lib/util/charset/codepoints.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/util/charset/codepoints.c b/lib/util/charset/codepoints.c index c39ed4a8436..b5220763d40 100644 --- a/lib/util/charset/codepoints.c +++ b/lib/util/charset/codepoints.c @@ -26,6 +26,7 @@ #include "dynconfig/dynconfig.h" #include "lib/util/debug.h" #include "lib/util/byteorder.h" +#include "lib/util/tsort.h" #ifdef strcasecmp #undef strcasecmp @@ -16483,7 +16484,7 @@ _PUBLIC_ int codepoint_cmpi(codepoint_t c1, codepoint_t c2) toupper_m(c1) == toupper_m(c2)) { return 0; } - return c1 - c2; + return NUMERIC_CMP(c1, c2); } -- 2.34.1 From 8d7bddf65701b0d80bbba68d99c72294a13aaa66 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:56:16 +1300 Subject: [PATCH 17/65] util:charset:codepoints: codepoint_cmpi warning about non-transitivity BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit f07ae6990702f8806c0c815454b80a5596b7219a) --- lib/util/charset/codepoints.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/util/charset/codepoints.c b/lib/util/charset/codepoints.c index b5220763d40..cc263745b6d 100644 --- a/lib/util/charset/codepoints.c +++ b/lib/util/charset/codepoints.c @@ -16480,6 +16480,18 @@ _PUBLIC_ bool isupper_m(codepoint_t val) */ _PUBLIC_ int codepoint_cmpi(codepoint_t c1, codepoint_t c2) { + /* + * FIXME: this is unsuitable for use in a sort, as the + * comparison is intransitive. + * + * The problem is toupper_m() is only called on equality case, + * which has strange effects. + * + * Consider {'a', 'A', 'B'}. + * 'a' == 'A' + * 'a' > 'B' (lowercase letters come after upper) + * 'A' < 'B' + */ if (c1 == c2 || toupper_m(c1) == toupper_m(c2)) { return 0; -- 2.34.1 From e363b61c003682143c4e74823e34e944f8c285b1 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 16:10:38 +1300 Subject: [PATCH 18/65] s3:libsmb:namequery: note intransitivity in addr_compare() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 4a9d274d43b1adac113419c649bbf530d180229d) --- source3/libsmb/namequery.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/source3/libsmb/namequery.c b/source3/libsmb/namequery.c index e6c0c7d2a09..01bac929776 100644 --- a/source3/libsmb/namequery.c +++ b/source3/libsmb/namequery.c @@ -1082,8 +1082,15 @@ bool name_status_find(const char *q_name, } /* - comparison function used by sort_addr_list -*/ + * comparison function used by sort_addr_list + * + * This comparison is intransitive in sort if a socket has an invalid + * family (i.e., not IPv4 or IPv6), or an interface doesn't support + * the family. Say we have sockaddrs with IP versions {4,5,6}, of + * which 5 is invalid. By this function, 4 == 5 and 6 == 5, but 4 != + * 6. This is of course a consequence of cmp() being unable to + * communicate error. + */ static int addr_compare(const struct sockaddr_storage *ss1, const struct sockaddr_storage *ss2) -- 2.34.1 From 188e870af05223d03b56d432e5d2163e5e7a1678 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 16:13:07 +1300 Subject: [PATCH 19/65] s3:libsmb:namequery: use NUMERIC_CMP in addr_compare This one was OK, as the numbers are tightly bound, but there is no real reason not to do it safely. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 3414a894ad6640fa8e282d650b1cc5319991545f) --- source3/libsmb/namequery.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source3/libsmb/namequery.c b/source3/libsmb/namequery.c index 01bac929776..8ae4004a98c 100644 --- a/source3/libsmb/namequery.c +++ b/source3/libsmb/namequery.c @@ -34,6 +34,7 @@ #include "lib/gencache.h" #include "librpc/gen_ndr/dns.h" #include "lib/util/util_net.h" +#include "lib/util/tsort.h" #include "lib/util/string_wrappers.h" /* nmbd.c sets this to True. */ @@ -1178,7 +1179,7 @@ static int addr_compare(const struct sockaddr_storage *ss1, max_bits2 += 128; } } - return max_bits2 - max_bits1; + return NUMERIC_CMP(max_bits2, max_bits1); } /* -- 2.34.1 From c75ef36f42722e78eab020f4477a5af4631d50ab Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 5 Apr 2024 14:22:11 +1300 Subject: [PATCH 20/65] lib/torture: add assert_int_{less,greater} macros In some situations, like comparison functions for qsort, we don't care about the actual value, just whethger it was greater or less than zero. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 6159b098cf35a8043682bfd4c4ea17ef0da6e8ee) --- lib/torture/torture.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/lib/torture/torture.h b/lib/torture/torture.h index 6818084ea96..3509911c8ca 100644 --- a/lib/torture/torture.h +++ b/lib/torture/torture.h @@ -525,6 +525,26 @@ static inline void torture_dump_data_str_cb(const char *buf, void *private_data) } \ } while(0) +#define torture_assert_int_less(torture_ctx,got,limit,cmt)\ + do { int __got = (got), __limit = (limit); \ + if (__got >= __limit) { \ + torture_result(torture_ctx, TORTURE_FAIL, \ + __location__": "#got" was %d (0x%X), expected < %d (0x%X): %s", \ + __got, __got, __limit, __limit, cmt); \ + return false; \ + } \ + } while(0) + +#define torture_assert_int_greater(torture_ctx,got,limit,cmt)\ + do { int __got = (got), __limit = (limit); \ + if (__got <= __limit) { \ + torture_result(torture_ctx, TORTURE_FAIL, \ + __location__": "#got" was %d (0x%X), expected > %d (0x%X): %s", \ + __got, __got, __limit, __limit, cmt); \ + return false; \ + } \ + } while(0) + #define torture_assert_int_equal_goto(torture_ctx,got,expected,ret,label,cmt)\ do { int __got = (got), __expected = (expected); \ if (__got != __expected) { \ -- 2.34.1 From f35ef61dcf6cedde4675af5345543e3b20544924 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 16:16:44 +1300 Subject: [PATCH 21/65] util: charset:util_str: use NUMERIC_CMP in strncasecmp_m_handle BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 997b72d79e651ddbc20e67006ae176229528dc6f) --- lib/util/charset/util_str.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/util/charset/util_str.c b/lib/util/charset/util_str.c index 04d69dfb428..c4db52160f4 100644 --- a/lib/util/charset/util_str.c +++ b/lib/util/charset/util_str.c @@ -157,14 +157,14 @@ _PUBLIC_ int strncasecmp_m_handle(struct smb_iconv_handle *iconv_handle, continue; } - return l1 - l2; + return NUMERIC_CMP(l1, l2); } if (n == 0) { return 0; } - return *s1 - *s2; + return NUMERIC_CMP(*s1, *s2); } /** -- 2.34.1 From 6bb474360ef98002b9f55d2313f59f0d79070245 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 17:32:48 +1300 Subject: [PATCH 22/65] ldb:attrib_handlers: ldb_comparison_Boolean uses NUMERIC_CMP() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit f78b964cd81db11097e78099c0699f571f20e126) --- lib/ldb/common/attrib_handlers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldb/common/attrib_handlers.c b/lib/ldb/common/attrib_handlers.c index 15470cfcc74..ce4c0a928e9 100644 --- a/lib/ldb/common/attrib_handlers.c +++ b/lib/ldb/common/attrib_handlers.c @@ -287,7 +287,7 @@ static int ldb_comparison_Boolean(struct ldb_context *ldb, void *mem_ctx, const struct ldb_val *v1, const struct ldb_val *v2) { if (v1->length != v2->length) { - return v1->length - v2->length; + return NUMERIC_CMP(v1->length, v2->length); } return strncasecmp((char *)v1->data, (char *)v2->data, v1->length); } -- 2.34.1 From dcc10624532fa214e221bae4965bd3ceaa88860e Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 17:43:03 +1300 Subject: [PATCH 23/65] ldb:attrib_handlers: ldb_comparison_binary uses NUMERIC_CMP() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 838c68470299045c5b1c9bdbd527edbeedebf2d6) --- lib/ldb/common/attrib_handlers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldb/common/attrib_handlers.c b/lib/ldb/common/attrib_handlers.c index ce4c0a928e9..baccf193f88 100644 --- a/lib/ldb/common/attrib_handlers.c +++ b/lib/ldb/common/attrib_handlers.c @@ -300,7 +300,7 @@ int ldb_comparison_binary(struct ldb_context *ldb, void *mem_ctx, const struct ldb_val *v1, const struct ldb_val *v2) { if (v1->length != v2->length) { - return v1->length - v2->length; + return NUMERIC_CMP(v1->length, v2->length); } return memcmp(v1->data, v2->data, v1->length); } -- 2.34.1 From ffabeba3dd1ada5d8f64c7ef6a57c7193accef1a Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 11:07:06 +1300 Subject: [PATCH 24/65] util:datablob: avoid non-transitive comparison in data_blob_cmp() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (backported from commit e1519c3667841ce27b15983eae378799ef9936f7) [dbagnall@samba.org: changed in master for conditional ACEs] --- lib/util/data_blob.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/util/data_blob.c b/lib/util/data_blob.c index 677f7c19211..687b6c8d3d5 100644 --- a/lib/util/data_blob.c +++ b/lib/util/data_blob.c @@ -22,6 +22,7 @@ #include "attr.h" #include "data_blob.h" #include "lib/util/samba_util.h" +#include "lib/util/tsort.h" const DATA_BLOB data_blob_null = { NULL, 0 }; @@ -121,11 +122,11 @@ _PUBLIC_ int data_blob_cmp(const DATA_BLOB *d1, const DATA_BLOB *d2) return 1; } if (d1->data == d2->data) { - return d1->length - d2->length; + return NUMERIC_CMP(d1->length, d2->length); } ret = memcmp(d1->data, d2->data, MIN(d1->length, d2->length)); if (ret == 0) { - return d1->length - d2->length; + return NUMERIC_CMP(d1->length, d2->length); } return ret; } -- 2.34.1 From 078af1c17e38aaf73cef8edc00115a151808cb5c Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 11:22:58 +1300 Subject: [PATCH 25/65] ldb: avoid non-transitive comparison in ldb_val_cmp() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 5c36bc82415b246fccec9eae693da82b7aa45b81) --- lib/ldb/common/ldb_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldb/common/ldb_msg.c b/lib/ldb/common/ldb_msg.c index 3242447b544..5c5f6a7c2b4 100644 --- a/lib/ldb/common/ldb_msg.c +++ b/lib/ldb/common/ldb_msg.c @@ -93,7 +93,7 @@ struct ldb_val *ldb_msg_find_val(const struct ldb_message_element *el, static int ldb_val_cmp(const struct ldb_val *v1, const struct ldb_val *v2) { if (v1->length != v2->length) { - return v1->length - v2->length; + return NUMERIC_CMP(v1->length, v2->length); } return memcmp(v1->data, v2->data, v1->length); } -- 2.34.1 From 75c8bcc2eddece1ec5722c41459ecb7705d6481b Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 11:26:25 +1300 Subject: [PATCH 26/65] ldb: reduce non-transitive comparisons in ldb_msg_element_compare() We can still have inconsistent comparisons, because two elements with the same number of values will always return -1 if they are unequal, which means they will sort differently depending on the order in which they are compared. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 21a071e4864dd739840c2ad4adb0c71ec33f8427) --- lib/ldb/common/ldb_msg.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/ldb/common/ldb_msg.c b/lib/ldb/common/ldb_msg.c index 5c5f6a7c2b4..e8164474882 100644 --- a/lib/ldb/common/ldb_msg.c +++ b/lib/ldb/common/ldb_msg.c @@ -749,9 +749,16 @@ int ldb_msg_element_compare(struct ldb_message_element *el1, unsigned int i; if (el1->num_values != el2->num_values) { - return el1->num_values - el2->num_values; + return NUMERIC_CMP(el1->num_values, el2->num_values); } - + /* + * Note this is an inconsistent comparison, unsuitable for + * sorting. If A has values {a, b} and B has values {b, c}, + * then + * + * ldb_msg_element_compare(A, B) returns -1, meaning A < B + * ldb_msg_element_compare(B, A) returns -1, meaning B < A + */ for (i=0;inum_values;i++) { if (!ldb_msg_find_val(el2, &el1->values[i])) { return -1; -- 2.34.1 From f7e1ea08c9da957bf1f06291a861c9a4091a5041 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 13:43:47 +1300 Subject: [PATCH 27/65] libcli/security: use NUMERIC_CMP in dom_sid_compare() sid->num_auths is always small (int8 < 16), so this is cosmetic only. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (backported from commit cb94202c1cf990e871ee2e8e43c577a0e4b9ee6f) [dbagnall@samba.org: file changed in master] --- libcli/security/dom_sid.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/libcli/security/dom_sid.c b/libcli/security/dom_sid.c index 9a91760ff62..38f98f9f906 100644 --- a/libcli/security/dom_sid.c +++ b/libcli/security/dom_sid.c @@ -28,6 +28,7 @@ #include "librpc/gen_ndr/security.h" #include "dom_sid.h" #include "lib/util/smb_strtox.h" +#include "lib/util/tsort.h" /***************************************************************** Compare the auth portion of two sids. @@ -71,12 +72,17 @@ int dom_sid_compare(const struct dom_sid *sid1, const struct dom_sid *sid2) return 1; /* Compare most likely different rids, first: i.e start at end */ - if (sid1->num_auths != sid2->num_auths) - return sid1->num_auths - sid2->num_auths; - - for (i = sid1->num_auths-1; i >= 0; --i) - if (sid1->sub_auths[i] != sid2->sub_auths[i]) - return sid1->sub_auths[i] - sid2->sub_auths[i]; + if (sid1->num_auths != sid2->num_auths) { + return NUMERIC_CMP(sid1->num_auths, sid2->num_auths); + } + for (i = sid1->num_auths-1; i >= 0; --i) { + if (sid1->sub_auths[i] < sid2->sub_auths[i]) { + return -1; + } + if (sid1->sub_auths[i] > sid2->sub_auths[i]) { + return 1; + } + } return dom_sid_compare_auth(sid1, sid2); } -- 2.34.1 From 2f50c93089ee3c0a7c25b8258b16a23b974c10d2 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 13:53:58 +1300 Subject: [PATCH 28/65] libcli/security: use NUMERIC_CMP in dom_sid_compare_auth() These numbers are all 8 bit, so overflow is unlikely. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 4641a97151783c2ae825582e91b4676d66dcb713) --- libcli/security/dom_sid.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libcli/security/dom_sid.c b/libcli/security/dom_sid.c index 38f98f9f906..737f4bb3a35 100644 --- a/libcli/security/dom_sid.c +++ b/libcli/security/dom_sid.c @@ -47,11 +47,12 @@ int dom_sid_compare_auth(const struct dom_sid *sid1, return 1; if (sid1->sid_rev_num != sid2->sid_rev_num) - return sid1->sid_rev_num - sid2->sid_rev_num; + return NUMERIC_CMP(sid1->sid_rev_num, sid2->sid_rev_num); for (i = 0; i < 6; i++) - if (sid1->id_auth[i] != sid2->id_auth[i]) - return sid1->id_auth[i] - sid2->id_auth[i]; + if (sid1->id_auth[i] != sid2->id_auth[i]) { + return NUMERIC_CMP(sid1->id_auth[i], sid2->id_auth[i]); + } return 0; } -- 2.34.1 From 14c4868ee5d527fd7c15d0f287b24d1dcb33ee0f Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:01:24 +1300 Subject: [PATCH 29/65] s3:lib:util_tdb: use NUMERIC_CMP() in tdb_data_cmp() Although these are size_t, in practice TDB data is limited to 32 bit. Even so, overflow of a signed int is possible. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit dd4a0c276813b2c8516061110a7e580aa9afcf40) --- source3/lib/util_tdb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source3/lib/util_tdb.c b/source3/lib/util_tdb.c index d85f676fbcf..3c7c1945f58 100644 --- a/source3/lib/util_tdb.c +++ b/source3/lib/util_tdb.c @@ -324,11 +324,11 @@ int tdb_data_cmp(TDB_DATA t1, TDB_DATA t2) return 1; } if (t1.dptr == t2.dptr) { - return t1.dsize - t2.dsize; + return NUMERIC_CMP(t1.dsize, t2.dsize); } ret = memcmp(t1.dptr, t2.dptr, MIN(t1.dsize, t2.dsize)); if (ret == 0) { - return t1.dsize - t2.dsize; + return NUMERIC_CMP(t1.dsize, t2.dsize); } return ret; } -- 2.34.1 From e993483fd7fd57976709550a010ed2e9a7f19f4b Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:10:45 +1300 Subject: [PATCH 30/65] s4:rpc_server: compare_SamEntry() uses NUMERIC_CMP() SamEntry.idx is uint32_t. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit ed3ab87bdb0f6c6a9ea6323ed240fe267220b759) --- source4/rpc_server/samr/dcesrv_samr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/rpc_server/samr/dcesrv_samr.c b/source4/rpc_server/samr/dcesrv_samr.c index 5570359728f..ba5be53d8c9 100644 --- a/source4/rpc_server/samr/dcesrv_samr.c +++ b/source4/rpc_server/samr/dcesrv_samr.c @@ -1166,7 +1166,7 @@ static NTSTATUS dcesrv_samr_CreateDomainGroup(struct dcesrv_call_state *dce_call */ static int compare_SamEntry(struct samr_SamEntry *e1, struct samr_SamEntry *e2) { - return e1->idx - e2->idx; + return NUMERIC_CMP(e1->idx, e2->idx); } static int compare_msgRid(struct ldb_message **m1, struct ldb_message **m2) { -- 2.34.1 From 43a5cff74da2c91232b743100b39a4d8df083c6c Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:22:24 +1300 Subject: [PATCH 31/65] s4:dns_server: use NUMERIC_CMP in rec_cmp() dnsp_DnssrvRpcRecord.dwTimeStamp is uint32_t, making overflow possible. dnsp_DnssrvRpcRecord.wType is an enum, which has the size of an int, though it may be hard to set it to overflowing values. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 42ead213484840121ce6bc0db22941ea0a019105) --- source4/dns_server/dnsserver_common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source4/dns_server/dnsserver_common.c b/source4/dns_server/dnsserver_common.c index 0bbba7ff6de..79468dbb8fe 100644 --- a/source4/dns_server/dnsserver_common.c +++ b/source4/dns_server/dnsserver_common.c @@ -642,7 +642,7 @@ static int rec_cmp(const struct dnsp_DnssrvRpcRecord *r1, * The records are sorted with higher types first, * which puts tombstones (type 0) last. */ - return r2->wType - r1->wType; + return NUMERIC_CMP(r2->wType, r1->wType); } /* * Then we need to sort from the oldest to newest timestamp. @@ -650,7 +650,7 @@ static int rec_cmp(const struct dnsp_DnssrvRpcRecord *r1, * Note that dwTimeStamp == 0 (never expiring) records come first, * then the ones whose expiry is soonest. */ - return r1->dwTimeStamp - r2->dwTimeStamp; + return NUMERIC_CMP(r1->dwTimeStamp, r2->dwTimeStamp); } /* -- 2.34.1 From 1b79988e4d89d735931cc289004271074b6531d9 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:54:09 +1300 Subject: [PATCH 32/65] s4:wins: use NUMERIC_CMP in winsdb_addr_sort_list() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 66d47537e42caa528c7fab670d9c35d27c513cce) --- source4/nbt_server/wins/winsdb.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source4/nbt_server/wins/winsdb.c b/source4/nbt_server/wins/winsdb.c index 2a05e96bca4..eb554fc5bd3 100644 --- a/source4/nbt_server/wins/winsdb.c +++ b/source4/nbt_server/wins/winsdb.c @@ -32,6 +32,7 @@ #include "lib/socket/netif.h" #include "param/param.h" #include "lib/util/smb_strtox.h" +#include "lib/util/tsort.h" #undef strcasecmp @@ -360,7 +361,7 @@ static int winsdb_addr_sort_list (struct winsdb_addr **p1, struct winsdb_addr ** a1_owned = true; } - return a2_owned - a1_owned; + return NUMERIC_CMP(a2_owned, a1_owned); } struct winsdb_addr **winsdb_addr_list_add(struct winsdb_handle *h, const struct winsdb_record *rec, -- 2.34.1 From 5cb32bdd6f76da3627e58c7808c88f9167e3de70 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:16:21 +1300 Subject: [PATCH 33/65] s4:wins: winsdb_addr_sort_list() uses NUMERIC_CMP() expire_time is time_t, which is at least int-sized, so overflow is possible (if this code ever runs). BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit d8b97649ef4d3ccaf53878021be0e2d4824b982c) --- source4/nbt_server/wins/winsdb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/nbt_server/wins/winsdb.c b/source4/nbt_server/wins/winsdb.c index eb554fc5bd3..7df40c33135 100644 --- a/source4/nbt_server/wins/winsdb.c +++ b/source4/nbt_server/wins/winsdb.c @@ -350,7 +350,7 @@ static int winsdb_addr_sort_list (struct winsdb_addr **p1, struct winsdb_addr ** * then the replica addresses with the newest to the oldest address */ if (a2->expire_time != a1->expire_time) { - return a2->expire_time - a1->expire_time; + return NUMERIC_CMP(a2->expire_time, a1->expire_time); } if (strcmp(a2->wins_owner, h->local_owner) == 0) { -- 2.34.1 From 5157bf3897f25de21c6815246f96e280064eb059 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:53:32 +1300 Subject: [PATCH 34/65] s4:wins: use NUMERIC_CMP in nbtd_wins_randomize1Clist_sort() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit a197be2003d7e248b1e1294f4ad5473f48762bce) --- source4/nbt_server/wins/winsserver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source4/nbt_server/wins/winsserver.c b/source4/nbt_server/wins/winsserver.c index a9f3ecd7d29..6679961dc03 100644 --- a/source4/nbt_server/wins/winsserver.c +++ b/source4/nbt_server/wins/winsserver.c @@ -36,6 +36,7 @@ #include "param/param.h" #include "libcli/resolve/resolve.h" #include "lib/util/util_net.h" +#include "lib/util/tsort.h" /* work out the ttl we will use given a client requested ttl @@ -653,7 +654,7 @@ static int nbtd_wins_randomize1Clist_sort(void *p1,/* (const char **) */ match_bits1 = ipv4_match_bits(interpret_addr2(a1), interpret_addr2(src->addr)); match_bits2 = ipv4_match_bits(interpret_addr2(a2), interpret_addr2(src->addr)); - return match_bits2 - match_bits1; + return NUMERIC_CMP(match_bits2, match_bits1); } static void nbtd_wins_randomize1Clist(struct loadparm_context *lp_ctx, -- 2.34.1 From c8095dc85851ed552be3b779223d9c7b0fcca90c Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:25:54 +1300 Subject: [PATCH 35/65] s3:util:net_registry: registry_value_cmp() uses NUMERIC_CMP() v->type is an int-sized enum, so overflow might be possible if it could be arbitrarily set. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 5e99262aaf5fc6601f3859c8b060b680b11bf6ea) --- source3/utils/net_registry.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/utils/net_registry.c b/source3/utils/net_registry.c index 5d1314ec37a..b47a8ff88b1 100644 --- a/source3/utils/net_registry.c +++ b/source3/utils/net_registry.c @@ -1146,7 +1146,7 @@ static int registry_value_cmp( if (v1->type == v2->type) { return data_blob_cmp(&v1->data, &v2->data); } - return v1->type - v2->type; + return NUMERIC_CMP(v1->type, v2->type); } static WERROR precheck_create_val(struct precheck_ctx *ctx, -- 2.34.1 From 0abf5fd537ee76fa1ea19b58db73ea1352fd5a11 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:56:48 +1300 Subject: [PATCH 36/65] s3:smbcacls: use NUMERIC_CMP in ace_compare BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 31101a9fa1503be9d8137e42466f57d85136a156) --- source3/utils/smbcacls.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/source3/utils/smbcacls.c b/source3/utils/smbcacls.c index 59913f63b11..afebcd40b32 100644 --- a/source3/utils/smbcacls.c +++ b/source3/utils/smbcacls.c @@ -506,22 +506,23 @@ static int ace_compare(struct security_ace *ace1, struct security_ace *ace2) return -1; if ((ace1->flags & SEC_ACE_FLAG_INHERITED_ACE) && (ace2->flags & SEC_ACE_FLAG_INHERITED_ACE)) - return ace1 - ace2; - - if (ace1->type != ace2->type) - return ace2->type - ace1->type; + return NUMERIC_CMP(ace1, ace2); + if (ace1->type != ace2->type) { + /* note the reverse order */ + return NUMERIC_CMP(ace2->type, ace1->type); + } if (dom_sid_compare(&ace1->trustee, &ace2->trustee)) return dom_sid_compare(&ace1->trustee, &ace2->trustee); if (ace1->flags != ace2->flags) - return ace1->flags - ace2->flags; + return NUMERIC_CMP(ace1->flags, ace2->flags); if (ace1->access_mask != ace2->access_mask) - return ace1->access_mask - ace2->access_mask; + return NUMERIC_CMP(ace1->access_mask, ace2->access_mask); if (ace1->size != ace2->size) - return ace1->size - ace2->size; + return NUMERIC_CMP(ace1->size, ace2->size); return memcmp(ace1, ace2, sizeof(struct security_ace)); } -- 2.34.1 From 58a7cc354dbdd65ae4bfe13669bc88a45f88b429 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:08:02 +1300 Subject: [PATCH 37/65] s3:util:sharesec ace_compare() uses NUMERIC_CMP() ace->access_mask is uint32_t, so can overflow a signed int. This would be easy to trigger, as it is a flags field rather than an allocation count. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit e35d54fd4d381df67ab9b4f8390e2109b2142678) --- source3/utils/sharesec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source3/utils/sharesec.c b/source3/utils/sharesec.c index 9b8064de702..8f6117209f4 100644 --- a/source3/utils/sharesec.c +++ b/source3/utils/sharesec.c @@ -119,19 +119,19 @@ static int ace_compare(struct security_ace *ace1, struct security_ace *ace2) return 0; if (ace1->type != ace2->type) - return ace2->type - ace1->type; + return NUMERIC_CMP(ace2->type, ace1->type); if (dom_sid_compare(&ace1->trustee, &ace2->trustee)) return dom_sid_compare(&ace1->trustee, &ace2->trustee); if (ace1->flags != ace2->flags) - return ace1->flags - ace2->flags; + return NUMERIC_CMP(ace1->flags, ace2->flags); if (ace1->access_mask != ace2->access_mask) - return ace1->access_mask - ace2->access_mask; + return NUMERIC_CMP(ace1->access_mask, ace2->access_mask); if (ace1->size != ace2->size) - return ace1->size - ace2->size; + return NUMERIC_CMP(ace1->size, ace2->size); return memcmp(ace1, ace2, sizeof(struct security_ace)); } -- 2.34.1 From f8e35367e4f5f40684c56f308128840ed035ad2e Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:33:47 +1300 Subject: [PATCH 38/65] s3:libsmb_xattr: ace_compare() uses NUMERIC_CMP() the access_mask is the easiest to overflow with subtraction -- other fields are 8 or 16 bit. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett Autobuild-User(master): Andrew Bartlett Autobuild-Date(master): Wed Apr 10 23:58:12 UTC 2024 on atb-devel-224 (cherry picked from commit 81598b42455d6758941da532c668b6d4e969cc40) --- source3/libsmb/libsmb_xattr.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/source3/libsmb/libsmb_xattr.c b/source3/libsmb/libsmb_xattr.c index 1f820521193..67cffe14971 100644 --- a/source3/libsmb/libsmb_xattr.c +++ b/source3/libsmb/libsmb_xattr.c @@ -121,7 +121,13 @@ ace_compare(struct security_ace *ace1, */ if (ace1->type != ace2->type) { - return ace2->type - ace1->type; + /* + * ace2 and ace1 are reversed here, so that + * ACCESS_DENIED_ACE_TYPE (1) sorts before + * ACCESS_ALLOWED_ACE_TYPE (0), which is the order you + * usually want. + */ + return NUMERIC_CMP(ace2->type, ace1->type); } if (dom_sid_compare(&ace1->trustee, &ace2->trustee)) { @@ -129,15 +135,15 @@ ace_compare(struct security_ace *ace1, } if (ace1->flags != ace2->flags) { - return ace1->flags - ace2->flags; + return NUMERIC_CMP(ace1->flags, ace2->flags); } if (ace1->access_mask != ace2->access_mask) { - return ace1->access_mask - ace2->access_mask; + return NUMERIC_CMP(ace1->access_mask, ace2->access_mask); } if (ace1->size != ace2->size) { - return ace1->size - ace2->size; + return NUMERIC_CMP(ace1->size, ace2->size); } return memcmp(ace1, ace2, sizeof(struct security_ace)); -- 2.34.1 From 7b44842e13ef5e99f52d89f4c8a873593149a29c Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 14:54:34 +1200 Subject: [PATCH 39/65] ldb:mod:sort: rearrange NULL checks There are further changes coming here. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit d785c1991c922150bab38c36cef3a799448ac304) --- lib/ldb/modules/sort.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/ldb/modules/sort.c b/lib/ldb/modules/sort.c index cb6f8df440f..8487c7003b6 100644 --- a/lib/ldb/modules/sort.c +++ b/lib/ldb/modules/sort.c @@ -121,15 +121,18 @@ static int sort_compare(struct ldb_message **msg1, struct ldb_message **msg2, vo el1 = ldb_msg_find_element(*msg1, ac->attributeName); el2 = ldb_msg_find_element(*msg2, ac->attributeName); - if (!el1 && el2) { + /* + * NULL elements sort at the end (regardless of ac->reverse flag). + */ + if (el1 == NULL && el2 == NULL) { + return 0; + } + if (el1 == NULL) { return 1; } - if (el1 && !el2) { + if (el2 == NULL) { return -1; } - if (!el1 && !el2) { - return 0; - } if (ac->reverse) return ac->a->syntax->comparison_fn(ldb, ac, &el2->values[0], &el1->values[0]); -- 2.34.1 From 7fab66071fc7d05aaa4b605b30609f6a171a58fc Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 14:55:27 +1200 Subject: [PATCH 40/65] ldb:sort: check that elements have values We assume no values is unlikely, since we have been dereferencing ->values[0] forever, with no known reports of trouble. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit d4e69734c65ade0bbb398447012513a7f27e98bd) --- lib/ldb/modules/sort.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/ldb/modules/sort.c b/lib/ldb/modules/sort.c index 8487c7003b6..a4a77329cee 100644 --- a/lib/ldb/modules/sort.c +++ b/lib/ldb/modules/sort.c @@ -122,7 +122,8 @@ static int sort_compare(struct ldb_message **msg1, struct ldb_message **msg2, vo el2 = ldb_msg_find_element(*msg2, ac->attributeName); /* - * NULL elements sort at the end (regardless of ac->reverse flag). + * NULL and empty elements sort at the end (regardless of ac->reverse flag). + * NULL elements come after empty ones. */ if (el1 == NULL && el2 == NULL) { return 0; @@ -133,6 +134,15 @@ static int sort_compare(struct ldb_message **msg1, struct ldb_message **msg2, vo if (el2 == NULL) { return -1; } + if (unlikely(el1->num_values == 0 && el2->num_values == 0)) { + return 0; + } + if (unlikely(el1->num_values == 0)) { + return 1; + } + if (unlikely(el2->num_values == 0)) { + return -1; + } if (ac->reverse) return ac->a->syntax->comparison_fn(ldb, ac, &el2->values[0], &el1->values[0]); -- 2.34.1 From 892577ae0eec4e56726af87e5b057ecbf56cd12c Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 14:58:48 +1200 Subject: [PATCH 41/65] ldb:sort: generalise both-NULL check to equality check BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 531f31df99341b2cb1afc42538022451ca771983) --- lib/ldb/modules/sort.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldb/modules/sort.c b/lib/ldb/modules/sort.c index a4a77329cee..72c60fc894a 100644 --- a/lib/ldb/modules/sort.c +++ b/lib/ldb/modules/sort.c @@ -125,7 +125,7 @@ static int sort_compare(struct ldb_message **msg1, struct ldb_message **msg2, vo * NULL and empty elements sort at the end (regardless of ac->reverse flag). * NULL elements come after empty ones. */ - if (el1 == NULL && el2 == NULL) { + if (el1 == el2) { return 0; } if (el1 == NULL) { -- 2.34.1 From aa06452a6a698485967d9dda2a2c1b77762e3125 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:04:43 +1200 Subject: [PATCH 42/65] ldb:dn: make ldb_dn_compare() self-consistent We were returning -1 in all these cases: ldb_dn_compare(dn, NULL); ldb_dn_compare(NULL, dn); ldb_dn_compare(NULL, NULL); which would give strange results in sort, where this is often used. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 5fe488d515a8bb719bdeafb8b64d8479732b5ac8) --- lib/ldb/common/ldb_dn.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/ldb/common/ldb_dn.c b/lib/ldb/common/ldb_dn.c index 7c0f0a2197b..92fa223ceb7 100644 --- a/lib/ldb/common/ldb_dn.c +++ b/lib/ldb/common/ldb_dn.c @@ -1132,8 +1132,22 @@ int ldb_dn_compare(struct ldb_dn *dn0, struct ldb_dn *dn1) { unsigned int i; int ret; + /* + * If used in sort, we shift NULL and invalid DNs to the end. + * + * If ldb_dn_casefold_internal() fails, that goes to the end too, so + * we end up with: + * + * | normal DNs, sorted | casefold failed DNs | invalid DNs | NULLs | + */ - if (( ! dn0) || dn0->invalid || ! dn1 || dn1->invalid) { + if (dn0 == dn1 || (dn0->invalid && dn1->invalid)) { + return 0; + } + if (dn0 == NULL || dn0->invalid) { + return 1; + } + if (dn1 == NULL || dn1->invalid) { return -1; } -- 2.34.1 From f584198fa24c618d2bc2cb27f003fa1aabae1d81 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:07:20 +1200 Subject: [PATCH 43/65] s3:brlock: use NUMERIC_CMP in #ifdef-zeroed lock_compare BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 9b73235d4957a487fbb3214fdfda6461a2cf0b21) --- source3/locking/brlock.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/source3/locking/brlock.c b/source3/locking/brlock.c index 905da049c58..328a9bfba3d 100644 --- a/source3/locking/brlock.c +++ b/source3/locking/brlock.c @@ -408,12 +408,9 @@ static int lock_compare(const struct lock_struct *lck1, const struct lock_struct *lck2) { if (lck1->start != lck2->start) { - return (lck1->start - lck2->start); + return NUMERIC_CMP(lck1->start, lck2->start); } - if (lck2->size != lck1->size) { - return ((int)lck1->size - (int)lck2->size); - } - return 0; + return NUMERIC_CMP(lck1->size, lck2->size); } #endif -- 2.34.1 From e355b7bef877455be7362166773c3c34dc568060 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:12:56 +1200 Subject: [PATCH 44/65] s3:mod:posixacl_xattr: use NUMERIC_CMP in posixacl_xattr_entry_compare The first subtraction was between uint16_t, so is safe with 32 bit int, but the second compared uint32_t, so was not safe. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 8b2605a5d9cc14f9e6ddf2db704cdca2f523d74e) --- source3/modules/posixacl_xattr.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source3/modules/posixacl_xattr.c b/source3/modules/posixacl_xattr.c index 365cdc79973..5d0516cf754 100644 --- a/source3/modules/posixacl_xattr.c +++ b/source3/modules/posixacl_xattr.c @@ -226,14 +226,14 @@ static int posixacl_xattr_entry_compare(const void *left, const void *right) tag_left = SVAL(left, 0); tag_right = SVAL(right, 0); - ret = (tag_left - tag_right); - if (!ret) { + ret = NUMERIC_CMP(tag_left, tag_right); + if (ret == 0) { /* ID is the third element in the entry, after two short integers (tag and perm), i.e at offset 4. */ id_left = IVAL(left, 4); id_right = IVAL(right, 4); - ret = id_left - id_right; + ret = NUMERIC_CMP(id_left, id_right); } return ret; -- 2.34.1 From 2d4fec261f8d2d2ca0735d6315ca9795559df350 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:17:22 +1200 Subject: [PATCH 45/65] s3:mod:vfs_vxfs: use NUMERIC_CMP in vxfs_ace_cmp BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 386216d4a158d8bafb0879a0a753da096a939b93) --- source3/modules/vfs_vxfs.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source3/modules/vfs_vxfs.c b/source3/modules/vfs_vxfs.c index c41d050e4ef..96db010226d 100644 --- a/source3/modules/vfs_vxfs.c +++ b/source3/modules/vfs_vxfs.c @@ -111,13 +111,13 @@ static int vxfs_ace_cmp(const void *ace1, const void *ace2) type_a1 = SVAL(ace1, 0); type_a2 = SVAL(ace2, 0); - ret = (type_a1 - type_a2); - if (!ret) { + ret = NUMERIC_CMP(type_a1, type_a2); + if (ret == 0) { /* Compare ID under type */ /* skip perm thus take offset as 4*/ id_a1 = IVAL(ace1, 4); id_a2 = IVAL(ace2, 4); - ret = id_a1 - id_a2; + ret = NUMERIC_CMP(id_a1, id_a2); } return ret; -- 2.34.1 From 1b8d503df9943e7cb6d6d4020f14daf082c10227 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:36:06 +1200 Subject: [PATCH 46/65] dsdb:schema: use NUMERIC_CMP in place of uint32_cmp uint32_cmp (introduced in 0c362597c0f933b3612bb17328c0a13b73d72e43 "fixed the sorting of schema attributes") was doing what NUMERIC_CMP does, but it was adding an extra function call. This results in less code. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 8317a6173646d425dc99e08bbf3d6086b0086bc5) --- source4/dsdb/schema/schema_set.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/source4/dsdb/schema/schema_set.c b/source4/dsdb/schema/schema_set.c index 398091c6375..8b90e7f7b7f 100644 --- a/source4/dsdb/schema/schema_set.c +++ b/source4/dsdb/schema/schema_set.c @@ -478,19 +478,13 @@ static void dsdb_setup_attribute_shortcuts(struct ldb_context *ldb, struct dsdb_ TALLOC_FREE(frame); } -static int uint32_cmp(uint32_t c1, uint32_t c2) -{ - if (c1 == c2) return 0; - return c1 > c2 ? 1 : -1; -} - static int dsdb_compare_class_by_lDAPDisplayName(struct dsdb_class **c1, struct dsdb_class **c2) { return strcasecmp((*c1)->lDAPDisplayName, (*c2)->lDAPDisplayName); } static int dsdb_compare_class_by_governsID_id(struct dsdb_class **c1, struct dsdb_class **c2) { - return uint32_cmp((*c1)->governsID_id, (*c2)->governsID_id); + return NUMERIC_CMP((*c1)->governsID_id, (*c2)->governsID_id); } static int dsdb_compare_class_by_governsID_oid(struct dsdb_class **c1, struct dsdb_class **c2) { @@ -507,11 +501,11 @@ static int dsdb_compare_attribute_by_lDAPDisplayName(struct dsdb_attribute **a1, } static int dsdb_compare_attribute_by_attributeID_id(struct dsdb_attribute **a1, struct dsdb_attribute **a2) { - return uint32_cmp((*a1)->attributeID_id, (*a2)->attributeID_id); + return NUMERIC_CMP((*a1)->attributeID_id, (*a2)->attributeID_id); } static int dsdb_compare_attribute_by_msDS_IntId(struct dsdb_attribute **a1, struct dsdb_attribute **a2) { - return uint32_cmp((*a1)->msDS_IntId, (*a2)->msDS_IntId); + return NUMERIC_CMP((*a1)->msDS_IntId, (*a2)->msDS_IntId); } static int dsdb_compare_attribute_by_attributeID_oid(struct dsdb_attribute **a1, struct dsdb_attribute **a2) { @@ -519,7 +513,7 @@ static int dsdb_compare_attribute_by_attributeID_oid(struct dsdb_attribute **a1, } static int dsdb_compare_attribute_by_linkID(struct dsdb_attribute **a1, struct dsdb_attribute **a2) { - return uint32_cmp((*a1)->linkID, (*a2)->linkID); + return NUMERIC_CMP((*a1)->linkID, (*a2)->linkID); } static int dsdb_compare_attribute_by_cn(struct dsdb_attribute **a1, struct dsdb_attribute **a2) { -- 2.34.1 From 1feab653362d1214789be13b66dba3f327816d14 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:47:12 +1200 Subject: [PATCH 47/65] s3:rpc:wkssvc_nt: dom_user_cmp uses NUMERIC_CMP usr->login_time is time_t, which is often bigger than int. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 75682e397b9cf22d04a5d80252554c6b2e376793) --- source3/rpc_server/wkssvc/srv_wkssvc_nt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/rpc_server/wkssvc/srv_wkssvc_nt.c b/source3/rpc_server/wkssvc/srv_wkssvc_nt.c index 592e8668cd3..89cdeb6daa1 100644 --- a/source3/rpc_server/wkssvc/srv_wkssvc_nt.c +++ b/source3/rpc_server/wkssvc/srv_wkssvc_nt.c @@ -140,7 +140,7 @@ static int dom_user_cmp(const struct dom_usr *usr1, const struct dom_usr *usr2) /* Called from qsort to compare two domain users in a dom_usr_t array * for sorting by login time. Return >0 if usr1 login time was later * than usr2 login time, <0 if it was earlier */ - return (usr1->login_time - usr2->login_time); + return NUMERIC_CMP(usr1->login_time, usr2->login_time); } /******************************************************************* -- 2.34.1 From 6474f5b13cb31855718a88604eb5c7bc8755a4b3 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:54:02 +1200 Subject: [PATCH 48/65] gensec: sort_gensec uses NUMERIC_CMP BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit acaa1323d0337ae9339dfff9f856ea54725a86ac) --- auth/gensec/gensec_start.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/auth/gensec/gensec_start.c b/auth/gensec/gensec_start.c index b6979812ac0..4511674c8eb 100644 --- a/auth/gensec/gensec_start.c +++ b/auth/gensec/gensec_start.c @@ -1103,7 +1103,7 @@ _PUBLIC_ const struct gensec_critical_sizes *gensec_interface_version(void) } static int sort_gensec(const struct gensec_security_ops **gs1, const struct gensec_security_ops **gs2) { - return (*gs2)->priority - (*gs1)->priority; + return NUMERIC_CMP((*gs2)->priority, (*gs1)->priority); } int gensec_setting_int(struct gensec_settings *settings, const char *mechanism, const char *name, int default_value) -- 2.34.1 From 68e45d44a2a88eb3c1b28923f53ea3e5cc1b541f Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Mon, 8 Apr 2024 17:06:57 +1200 Subject: [PATCH 49/65] lib/socket: rearrange iface_comp() to use NUMERIC_CMP We rearrange rather than just replacing the subtraction, because that would call ntohl() more than necessary, and I think the flow is a bit clearer this way. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 7ba6fcb93656e5e88e1d5bcd6002747aa64f0a3a) --- lib/socket/interfaces.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/socket/interfaces.c b/lib/socket/interfaces.c index d8724e6c669..76a6570696c 100644 --- a/lib/socket/interfaces.c +++ b/lib/socket/interfaces.c @@ -386,18 +386,18 @@ static int iface_comp(struct iface_struct *i1, struct iface_struct *i2) if (((struct sockaddr *)&i1->ip)->sa_family == AF_INET) { struct sockaddr_in *s1 = (struct sockaddr_in *)&i1->ip; struct sockaddr_in *s2 = (struct sockaddr_in *)&i2->ip; - - r = ntohl(s1->sin_addr.s_addr) - - ntohl(s2->sin_addr.s_addr); - if (r) { - return r; + uint32_t a1 = ntohl(s1->sin_addr.s_addr); + uint32_t a2 = ntohl(s2->sin_addr.s_addr); + r = NUMERIC_CMP(a1, a2); + if (r == 0) { + /* compare netmasks as a tiebreaker */ + s1 = (struct sockaddr_in *)&i1->netmask; + s2 = (struct sockaddr_in *)&i2->netmask; + a1 = ntohl(s1->sin_addr.s_addr); + a2 = ntohl(s2->sin_addr.s_addr); + r = NUMERIC_CMP(a1, a2); } - - s1 = (struct sockaddr_in *)&i1->netmask; - s2 = (struct sockaddr_in *)&i2->netmask; - - return ntohl(s1->sin_addr.s_addr) - - ntohl(s2->sin_addr.s_addr); + return r; } return 0; } -- 2.34.1 From 04bc1faf3e0808a663d2e699eee263b5022170f5 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Mon, 8 Apr 2024 17:08:03 +1200 Subject: [PATCH 50/65] s3:libsmb:nmblib: use NUMERIC_CMP in status_compare BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 31c322874b8b65518cec945e05a42fd014e6390b) --- source3/libsmb/nmblib.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source3/libsmb/nmblib.c b/source3/libsmb/nmblib.c index c90e92ebb69..ea4c5b7b35a 100644 --- a/source3/libsmb/nmblib.c +++ b/source3/libsmb/nmblib.c @@ -1229,8 +1229,10 @@ static unsigned char sort_ip[4]; static int name_query_comp(unsigned char *p1, unsigned char *p2) { - return matching_len_bits(p2+2, sort_ip, 4) - - matching_len_bits(p1+2, sort_ip, 4); + int a = matching_len_bits(p1+2, sort_ip, 4); + int b = matching_len_bits(p2+2, sort_ip, 4); + /* reverse sort -- p2 derived value comes first */ + return NUMERIC_CMP(b, a); } /**************************************************************************** -- 2.34.1 From 3e0ccaa52c901d0a6fa712a5faff813e0fae0e6c Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Mon, 8 Apr 2024 22:54:49 +1200 Subject: [PATCH 51/65] s4:rpcsrv:dnsserver: make dns_name_compare transitive with NULLs Returning 0 on `(name1 == NULL || name2 == NULL)` made NULL equal to everything, which confuses a sort (consider {A, B, NULL} where A > B, but A == NULL == B). The only caller is dnsserver_enumerate_records() which fails if it finds a NULL in the sorted list. We make the happen more quickly by sorting NULLs to the front. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 7be535315a5eed5d5b7eaea025ecf9f55e772e8e) --- source4/rpc_server/dnsserver/dnsdata.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/source4/rpc_server/dnsserver/dnsdata.c b/source4/rpc_server/dnsserver/dnsdata.c index 002d9e622cc..4b2c04a089e 100644 --- a/source4/rpc_server/dnsserver/dnsdata.c +++ b/source4/rpc_server/dnsserver/dnsdata.c @@ -1075,9 +1075,23 @@ int dns_name_compare(struct ldb_message * const *m1, struct ldb_message * const name1 = ldb_msg_find_attr_as_string(*m1, "name", NULL); name2 = ldb_msg_find_attr_as_string(*m2, "name", NULL); - if (name1 == NULL || name2 == NULL) { + /* + * We sort NULL names to the start of the list, because the only + * caller of this function, dnsserver_enumerate_records() will call + * dns_build_tree() with the sorted list, which will always return an + * error when it hits a NULL, so we might as well make that happen + * quickly. + */ + if (name1 == name2) { + /* this includes the both NULL case */ return 0; } + if (name1 == NULL) { + return -1; + } + if (name2 == NULL) { + return 1; + } /* Compare the last components of names. * If search_name is not NULL, compare the second last components of names */ -- 2.34.1 From 0754d424cef9023a4fbbcae4f9de006bce0230d5 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Mon, 8 Apr 2024 22:55:50 +1200 Subject: [PATCH 52/65] s4:rpcsrv:samr: improve a comment in compare_msgRid BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 6229feab74a734190c302ee9b1cc36960669743d) --- source4/rpc_server/samr/dcesrv_samr.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/source4/rpc_server/samr/dcesrv_samr.c b/source4/rpc_server/samr/dcesrv_samr.c index ba5be53d8c9..8faa457e80f 100644 --- a/source4/rpc_server/samr/dcesrv_samr.c +++ b/source4/rpc_server/samr/dcesrv_samr.c @@ -1197,8 +1197,9 @@ static int compare_msgRid(struct ldb_message **m1, struct ldb_message **m2) { } /* - * Get and compare the rids, if we fail to extract a rid treat it as a - * missing SID and sort to the end of the list + * Get and compare the rids. If we fail to extract a rid (because + * there are no subauths) the msg goes to the end of the list, but + * before the NULL SIDs. */ status = dom_sid_split_rid(NULL, sid1, NULL, &rid1); if (!NT_STATUS_IS_OK(status)) { -- 2.34.1 From 6526ec1249ff22461af7f6a372b3d65b31739653 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 10 Apr 2024 10:54:31 +1200 Subject: [PATCH 53/65] ldb-samba: ldif-handlers: make ldif_comparison_objectSid() accurate This function compares blobs that might be SID strings or might be SID structures. Until now, if they were both (seemingly) strings, they were compared as strings, otherwise if either was a string it was converted to a structure blob, then the blobs were compared. This had two big problems: 1. There is variety in the way a SID can be stringified. For example, "s-1-02-3" means the same SID as "S-1-2-3", but those wouldn't compare equal. 2. SID comparison was crazily non-transitive. Consider the three values a = "S-1-2-3-4-5", b = "S-1-9-1", c = SID("S-1-11-1"), where c is a struct and the others are string. then we had, a < b, because the 5th character '2' < '9'. a > c, because when converted to a structure, the number of sub-auths is the first varying byte. a has 3, c has 0. b < c, because after the sub-auth count comes the id_auth value (big-endian, which doesn't matter in this case). That made the function unreliable for sorting, AND for simple equality tests. Also it leaked. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 6722e80d1b3a252a1ed714be4a35185cd99971e3) --- lib/ldb-samba/ldif_handlers.c | 59 +++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 24 deletions(-) diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c index f77a268c1a8..52244971931 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -150,36 +150,47 @@ bool ldif_comparision_objectSid_isString(const struct ldb_val *v) /* compare two objectSids + + If the SIDs seem to be strings, they are converted to binary form. */ static int ldif_comparison_objectSid(struct ldb_context *ldb, void *mem_ctx, const struct ldb_val *v1, const struct ldb_val *v2) { - if (ldif_comparision_objectSid_isString(v1) && ldif_comparision_objectSid_isString(v2)) { - return ldb_comparison_binary(ldb, mem_ctx, v1, v2); - } else if (ldif_comparision_objectSid_isString(v1) - && !ldif_comparision_objectSid_isString(v2)) { - struct ldb_val v; - int ret; - if (ldif_read_objectSid(ldb, mem_ctx, v1, &v) != 0) { - /* Perhaps not a string after all */ - return ldb_comparison_binary(ldb, mem_ctx, v1, v2); + bool v1_is_string = ldif_comparision_objectSid_isString(v1); + bool v2_is_string = ldif_comparision_objectSid_isString(v2); + struct ldb_val parsed_1 = {}; + struct ldb_val parsed_2 = {}; + int ret; + /* + * If the ldb_vals look like SID strings (i.e. start with "S-" + * or "s-"), we try to parse them as such. If that fails, we + * assume they are binary SIDs, even though that's not really + * possible -- the first two bytes of a struct dom_sid are the + * version (1), and the number of sub-auths (<= 15), neither + * of which are close to 'S' or '-'. + */ + if (v1_is_string) { + int r = ldif_read_objectSid(ldb, mem_ctx, v1, &parsed_1); + if (r == 0) { + v1 = &parsed_1; } - ret = ldb_comparison_binary(ldb, mem_ctx, &v, v2); - talloc_free(v.data); - return ret; - } else if (!ldif_comparision_objectSid_isString(v1) - && ldif_comparision_objectSid_isString(v2)) { - struct ldb_val v; - int ret; - if (ldif_read_objectSid(ldb, mem_ctx, v2, &v) != 0) { - /* Perhaps not a string after all */ - return ldb_comparison_binary(ldb, mem_ctx, v1, v2); - } - ret = ldb_comparison_binary(ldb, mem_ctx, v1, &v); - talloc_free(v.data); - return ret; } - return ldb_comparison_binary(ldb, mem_ctx, v1, v2); + if (v2_is_string) { + int r = ldif_read_objectSid(ldb, mem_ctx, v2, &parsed_2); + if (r == 0) { + v2 = &parsed_2; + } + } + + ret = ldb_comparison_binary(ldb, mem_ctx, v1, v2); + + if (v1_is_string) { + TALLOC_FREE(parsed_1.data); + } + if (v2_is_string) { + TALLOC_FREE(parsed_2.data); + } + return ret; } /* -- 2.34.1 From aea663475266620c01617a72bda55ed94825cd69 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Apr 2024 16:25:02 +1200 Subject: [PATCH 54/65] ldb-samba:ldif_handlers: dn_link_comparison semi-sorts deleted objects We were always returning -1 for a deleted object, which works for an equality test, but not a relative comparison. This sorts deleted DNs toward the end of the list -- except when both DNs are deleted. What should happen there is yet to be determined. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit db963b1674ede357d4edba578e0e0372dcb2f287) --- lib/ldb-samba/ldif_handlers.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c index 52244971931..896315de44d 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1159,13 +1159,15 @@ static int samba_ldb_dn_link_comparison(struct ldb_context *ldb, void *mem_ctx, struct ldb_dn *dn1 = NULL, *dn2 = NULL; int ret; + /* + * In a sort context, Deleted DNs get shifted to the end. + * They never match in an equality + */ if (dsdb_dn_is_deleted_val(v1)) { - /* If the DN is deleted, then we can't search for it */ - return -1; + return 1; } if (dsdb_dn_is_deleted_val(v2)) { - /* If the DN is deleted, then we can't search for it */ return -1; } -- 2.34.1 From b29331270ab486dde8ed929ff330d1a8a191829d Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Apr 2024 16:26:03 +1200 Subject: [PATCH 55/65] ldb-samba:ldif_handlers: dn_link_comparison semi-sorts invalid DNs these tend to go to the end of the sorted array. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 11d5a809325369b48d14023adf109e418bb1c7af) --- lib/ldb-samba/ldif_handlers.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c index 896315de44d..11fc242eba3 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1172,7 +1172,9 @@ static int samba_ldb_dn_link_comparison(struct ldb_context *ldb, void *mem_ctx, } dn1 = ldb_dn_from_ldb_val(mem_ctx, ldb, v1); - if ( ! ldb_dn_validate(dn1)) return -1; + if ( ! ldb_dn_validate(dn1)) { + return 1; + } dn2 = ldb_dn_from_ldb_val(mem_ctx, ldb, v2); if ( ! ldb_dn_validate(dn2)) { -- 2.34.1 From 98320cc678e9df78c5ab9e9966e77e8ee9573005 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Apr 2024 16:53:03 +1200 Subject: [PATCH 56/65] ldb-samba:ldif_handlers: dn_link_comparison correctly sorts deleted objects This changes the behaviour of the DN syntax .comparison_fn when being used in a search, if the search key is a deleted DN. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 70356592563bf758dbe509413445b77bb0d7da14) --- lib/ldb-samba/ldif_handlers.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c index 11fc242eba3..df5a23c4458 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1164,10 +1164,17 @@ static int samba_ldb_dn_link_comparison(struct ldb_context *ldb, void *mem_ctx, * They never match in an equality */ if (dsdb_dn_is_deleted_val(v1)) { - return 1; - } - - if (dsdb_dn_is_deleted_val(v2)) { + if (! dsdb_dn_is_deleted_val(v2)) { + return 1; + } + /* + * They are both deleted! + * + * The soundest thing to do at this point is carry on + * and compare the DNs normally. This matches the + * behaviour of samba_dn_extended_match() below. + */ + } else if (dsdb_dn_is_deleted_val(v2)) { return -1; } -- 2.34.1 From 3aec5014968535ee6839526f4664dac031bf6f3e Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Apr 2024 16:59:50 +1200 Subject: [PATCH 57/65] ldb-samba:ldif_handlers: dn_link_comparison leaks less dn1 and dn2 can be invalid but still occupying memory. (ldb_dn_validate(dn2) does contain a NULL check, but a lot more besides). BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 341b8fb60e291ad598fafd7a09a75e9b249de07f) --- lib/ldb-samba/ldif_handlers.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c index df5a23c4458..90973cbf3c3 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1180,12 +1180,14 @@ static int samba_ldb_dn_link_comparison(struct ldb_context *ldb, void *mem_ctx, dn1 = ldb_dn_from_ldb_val(mem_ctx, ldb, v1); if ( ! ldb_dn_validate(dn1)) { + TALLOC_FREE(dn1); return 1; } dn2 = ldb_dn_from_ldb_val(mem_ctx, ldb, v2); if ( ! ldb_dn_validate(dn2)) { - talloc_free(dn1); + TALLOC_FREE(dn1); + TALLOC_FREE(dn2); return -1; } -- 2.34.1 From 97c98c59f2b0cb3907614e9a02f92115904654ce Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Apr 2024 18:08:54 +1200 Subject: [PATCH 58/65] ldb-samba:ldif_handlers: dn_link_comparison: sort invalid DNs If both DNs are invalid, we can say they are equal. This means invalid or NULL DNs will sort to the end of the array, before deleted DNs: [ valid DNs, sorted | invalid/NULL DNs | deleted DNs, sorted ] BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 7280c8e53f463108fe3de443ce63572dde689a30) --- lib/ldb-samba/ldif_handlers.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c index 90973cbf3c3..e339c1c8b10 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1179,12 +1179,18 @@ static int samba_ldb_dn_link_comparison(struct ldb_context *ldb, void *mem_ctx, } dn1 = ldb_dn_from_ldb_val(mem_ctx, ldb, v1); + dn2 = ldb_dn_from_ldb_val(mem_ctx, ldb, v2); + if ( ! ldb_dn_validate(dn1)) { TALLOC_FREE(dn1); + if ( ! ldb_dn_validate(dn2)) { + TALLOC_FREE(dn2); + return 0; + } + TALLOC_FREE(dn2); return 1; } - dn2 = ldb_dn_from_ldb_val(mem_ctx, ldb, v2); if ( ! ldb_dn_validate(dn2)) { TALLOC_FREE(dn1); TALLOC_FREE(dn2); -- 2.34.1 From d46c288be5ed63c6cc575476650bbc692c6e6775 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 10 Apr 2024 10:54:41 +1200 Subject: [PATCH 59/65] ldb:attrib_handlers: make ldb_comparison_Boolean more consistent This isn't supposed to be used for sorting, but it is hard to say it won't be, so we might as well make it sort properly. Following long-standing behaviour, we try to sort "FALSE" > "TRUE", by length, then switch to using strncasecmp(). strncasecmp would sort the other way, so we swap the operands. This is to make e.g. "TRUE\0" sort the same as "TRUE". BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit a75c98ad688415aec8afc617a759ba90cfd9f23b) --- lib/ldb/common/attrib_handlers.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/lib/ldb/common/attrib_handlers.c b/lib/ldb/common/attrib_handlers.c index baccf193f88..c01477331f0 100644 --- a/lib/ldb/common/attrib_handlers.c +++ b/lib/ldb/common/attrib_handlers.c @@ -281,15 +281,36 @@ static int ldb_canonicalise_Boolean(struct ldb_context *ldb, void *mem_ctx, } /* - compare two Booleans -*/ + * compare two Booleans. + * + * According to RFC4517 4.2.2, "the booleanMatch rule is an equality matching + * rule", meaning it isn't used for ordering. + * + * However, it seems conceivable that Samba could be coerced into sorting on a + * field with Boolean syntax, so we might as well have consistent behaviour in + * that case. + * + * The most probably values are {"FALSE", 5} and {"TRUE", 4}. To save time we + * compare first by length, which makes FALSE > TRUE. This is somewhat + * contrary to convention, but is how Samba has worked forever. + * + * If somehow we are comparing incompletely normalised values where the length + * is the same (for example {"false", 5} and {"TRUE\0", 5}), the length is the + * same, and we fall back to a strncasecmp. In this case, since "FALSE" is + * alphabetically lower, we swap the order, so that "TRUE\0" again comes + * before "FALSE". + * + * ldb_canonicalise_Boolean (just above) gives us a clue as to what we might + * expect to cope with by way of invalid values. + */ static int ldb_comparison_Boolean(struct ldb_context *ldb, void *mem_ctx, const struct ldb_val *v1, const struct ldb_val *v2) { if (v1->length != v2->length) { - return NUMERIC_CMP(v1->length, v2->length); + return NUMERIC_CMP(v2->length, v1->length); } - return strncasecmp((char *)v1->data, (char *)v2->data, v1->length); + /* reversed, see long comment above */ + return strncasecmp((char *)v2->data, (char *)v1->data, v1->length); } -- 2.34.1 From 71397002d0ad2da30571b4faaded32daddbe8880 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 26 Apr 2024 15:24:47 +1200 Subject: [PATCH 60/65] ldb: avoid NULL deref in ldb_db_compare This also sorts NULLs after invalid DNs, which matches the comment above. CID 1596622. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit af7654331fb6a2d9cc41cf5bdffa74c81ff4ffee) --- lib/ldb/common/ldb_dn.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/ldb/common/ldb_dn.c b/lib/ldb/common/ldb_dn.c index 92fa223ceb7..8388fdb7318 100644 --- a/lib/ldb/common/ldb_dn.c +++ b/lib/ldb/common/ldb_dn.c @@ -1141,13 +1141,23 @@ int ldb_dn_compare(struct ldb_dn *dn0, struct ldb_dn *dn1) * | normal DNs, sorted | casefold failed DNs | invalid DNs | NULLs | */ - if (dn0 == dn1 || (dn0->invalid && dn1->invalid)) { + if (dn0 == dn1) { + /* this includes the both-NULL case */ return 0; } - if (dn0 == NULL || dn0->invalid) { + if (dn0 == NULL) { return 1; } - if (dn1 == NULL || dn1->invalid) { + if (dn1 == NULL) { + return -1; + } + if (dn0->invalid && dn1->invalid) { + return 0; + } + if (dn0->invalid) { + return 1; + } + if (dn1->invalid) { return -1; } -- 2.34.1 From 1df1b5f15237806c6b26c3a52fa171b53ab856f0 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 12 Apr 2024 18:11:47 +1200 Subject: [PATCH 61/65] s4:dsdb:mod: repl_md: make message_sort transitive Before we had (with a TODO of regret): if (!a1 || !a2) { return strcasecmp(e1->name, e2->name); } so, given {name:"A", id 2}, {name:"B", NO id}, {name:"C", id 1}, A < B by name B < C by name A > C by id Now the sort order is always A > C > B. This sort could have caused mysterious crashes in repl_meta_data if the schema is out of sync. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 5335f122fb551231a02a58f88f6a0aa23b5e02cb) --- source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index 1975c01b91d..762fe69f6c5 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -1063,12 +1063,20 @@ static int replmd_ldb_message_element_attid_sort(const struct ldb_message_elemen a2 = dsdb_attribute_by_lDAPDisplayName(schema, e2->name); /* - * TODO: remove this check, we should rely on e1 and e2 having valid attribute names - * in the schema + * If the elements do not have valid attribute names in the schema + * (which we would prefer to think can't happen), we need to sort them + * somehow. The current strategy is to put them at the end, sorted by + * attribute name. */ - if (!a1 || !a2) { + if (a1 == NULL && a2 == NULL) { return strcasecmp(e1->name, e2->name); } + if (a1 == NULL) { + return 1; + } + if (a2 == NULL) { + return -1; + } if (a1->attributeID_id == a2->attributeID_id) { return 0; } -- 2.34.1 From b21e8864c80605cabbb8bcdfa85616527164fed6 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 12 Apr 2024 20:28:04 +1200 Subject: [PATCH 62/65] s4:dsdb:mod: repl_md: message sort uses NUMERIC_CMP() No change at all in the result, just saving lines and branches. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 827b0c39ed0497407bfcfc5683735a165b1b0f0a) --- source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index 762fe69f6c5..4f2aefe7662 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -1077,10 +1077,7 @@ static int replmd_ldb_message_element_attid_sort(const struct ldb_message_elemen if (a2 == NULL) { return -1; } - if (a1->attributeID_id == a2->attributeID_id) { - return 0; - } - return a1->attributeID_id > a2->attributeID_id ? 1 : -1; + return NUMERIC_CMP(a1->attributeID_id, a2->attributeID_id); } static void replmd_ldb_message_sort(struct ldb_message *msg, -- 2.34.1 From eb663aa0813f868a14b2004d0386ed5f8079bc23 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Apr 2024 13:21:25 +1200 Subject: [PATCH 63/65] ldb:attrib_handlers: use NUMERIC_CMP in ldb_comparison_fold BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit f81b7c7eb206a447d799a25cc2da26304dc7567a) --- lib/ldb/common/attrib_handlers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldb/common/attrib_handlers.c b/lib/ldb/common/attrib_handlers.c index c01477331f0..75838b4e409 100644 --- a/lib/ldb/common/attrib_handlers.c +++ b/lib/ldb/common/attrib_handlers.c @@ -425,7 +425,7 @@ utf8str: while (*u1 == ' ') u1++; while (*u2 == ' ') u2++; } - ret = (int)(*u1 - *u2); + ret = NUMERIC_CMP(*u1, *u2); talloc_free(b1); talloc_free(b2); -- 2.34.1 From a40f666a8266396fc6aaefbd889db16755956e86 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 26 Apr 2024 15:58:44 +1200 Subject: [PATCH 64/65] ldb:attrib_handlers: reduce non-transitive behaviour in ldb_comparison_fold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If two strings are invalid UTF-8, the string is first compared with memcmp(), which compares as unsigned char. If the strings are of different lengths and one is a substring of the other, the memcmp() returns 0 and a second comparison is made which assumes the next character in the shorter string is '\0' -- but this comparison was done using SIGNED chars (on most systems). That leads to non-transitive comparisons. Consider the strings {"a\xff", "a", "ab\xff"} under that system. "a\xff" < "a", because (char)0xff == -1. "ab\xff" > "a", because 'b' == 98. "ab\xff" < "a\xff", because memcmp("ab\xff", "a\xff", 2) avoiding the signed char tiebreaker. (Before c49c48afe09a1a78989628bbffd49dd3efc154dd, the final character might br arbitrarily cast into another character -- in latin-1, for example, the 0xff here would have been seen as 'ÿ', which would be uppercased to 'Ÿ', which is U+0178, which would be truncated to '\x78', a positive char. On the other hand e.g. 0xfe, 'þ', would have mapped to 0xde, 'Þ', remaining negative). BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit e2051eebd492a419f840280336eb242d0b4a26ac) --- lib/ldb/common/attrib_handlers.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/ldb/common/attrib_handlers.c b/lib/ldb/common/attrib_handlers.c index 75838b4e409..3d13e4bd9fd 100644 --- a/lib/ldb/common/attrib_handlers.c +++ b/lib/ldb/common/attrib_handlers.c @@ -393,17 +393,27 @@ utf8str: b2 = ldb_casefold(ldb, mem_ctx, s2, n2); if (!b1 || !b2) { - /* One of the strings was not UTF8, so we have no - * options but to do a binary compare */ + /* + * One of the strings was not UTF8, so we have no + * options but to do a binary compare. + */ talloc_free(b1); talloc_free(b2); ret = memcmp(s1, s2, MIN(n1, n2)); if (ret == 0) { - if (n1 == n2) return 0; + if (n1 == n2) { + return 0; + } if (n1 > n2) { - return (int)ldb_ascii_toupper(s1[n2]); + if (s1[n2] == '\0') { + return 0; + } + return 1; } else { - return -(int)ldb_ascii_toupper(s2[n1]); + if (s2[n1] == '\0') { + return 0; + } + return -1; } } return ret; -- 2.34.1 From d1234168fefd92ba0aba9d6245b7eabdb59812eb Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Tue, 21 May 2024 20:20:36 +1200 Subject: [PATCH 65/65] s3:smbcacls: fix ace_compare We got this wrong in commit 31101a9fa1503be9d8137e42466f57d85136a156. In fact, we should probably not reorder the inherited ACLs, but that is for another patch series. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 5abeb724d74af2b861f2ee6bc27762bb5bf07bca) --- source3/utils/smbcacls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/utils/smbcacls.c b/source3/utils/smbcacls.c index afebcd40b32..b9d46b21d36 100644 --- a/source3/utils/smbcacls.c +++ b/source3/utils/smbcacls.c @@ -506,7 +506,7 @@ static int ace_compare(struct security_ace *ace1, struct security_ace *ace2) return -1; if ((ace1->flags & SEC_ACE_FLAG_INHERITED_ACE) && (ace2->flags & SEC_ACE_FLAG_INHERITED_ACE)) - return NUMERIC_CMP(ace1, ace2); + return NUMERIC_CMP(ace2->type, ace1->type); if (ace1->type != ace2->type) { /* note the reverse order */ -- 2.34.1