From 25470ef8c046991aae42b9cc39fa5cd029cf522b 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 4087a3c554ef10567a70e319d5809acb3a77d464 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 1313528e3731f594c360f6daf017e0c8a59f514e 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 f0cdfe66a59ec8224502e154ec437e79df504b7e 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 5d83a270573..98859d47b9a 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 27886613193fa6b0056e1bd8a19452abcad65329 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 1eb86e00ff95d54e3eaa83c1661c05ed484104b0 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 bc83241dd7865326353c2861839e1aab20e65896 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 92102371674..d2d5eed1354 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 bb715ed74a3518bdadc9ba44b3bfb93ed6cc2bde 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 1317b58874c..20613a75384 100644 --- a/source4/dsdb/samdb/ldb_modules/operational.c +++ b/source4/dsdb/samdb/ldb_modules/operational.c @@ -1070,7 +1070,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 7f3f844b4a8ea8895d62ec085defec48e7e9e8c8 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 aba7f4128b2..ccbc74c671c 100644 --- a/source4/dns_server/dnsserver_common.c +++ b/source4/dns_server/dnsserver_common.c @@ -1408,7 +1408,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 b087f6151724c74eb46d33ac81ad57fbef1ebefe 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 b3a33c6aca69d86481cfbba94ff22ab98345dfc8 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 b2f2aa83cc29213b29305c40fd2391098958448a 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 3af43af4c42f1202617f5a602875a0d6cdf53a99 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 744ab4fa622423d107438f379aa0fdf386b95a71 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 1650c9b8232..bd9cd6e69f5 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 978e7713e2838acbd73a6512dad40ae0696dd6e2 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 50e280ac9e6e034126bcb08239df98f7c713359d 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 ea2c4be7fe6..68b7b08ee50 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 6ddf896280027f4536a23b0afe615a39f1e27c8e 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 68b7b08ee50..80226278faf 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 0097efa3c707e71025af50e042ae68b4b20d7198 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 73a552a553a..f787e516da0 100644 --- a/source3/libsmb/namequery.c +++ b/source3/libsmb/namequery.c @@ -1087,8 +1087,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 b0cf1729c206a6906cb79a830005d7854591c265 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 f787e516da0..8f6a9b51f81 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. */ @@ -1183,7 +1184,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 83da0dbb79b1fb7a437e92bf62c6dd7467c0a40d 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 2e86e3173cc..2194703d5fc 100644 --- a/lib/torture/torture.h +++ b/lib/torture/torture.h @@ -534,6 +534,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 19b9110a1b1e491c5c3e156998527e318f95acc3 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 bd9cd6e69f5..c52b77384ce 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 7f92dc7e5b309be28727907c9e0a2522bc37fba2 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 927b21478361725147b8fed8f17884ad601ee057 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 26a419b7964be680d20442cb30599db4df6f9336 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 (cherry picked from commit e1519c3667841ce27b15983eae378799ef9936f7) --- 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 69a340c6fb8..15582000205 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,12 +122,12 @@ _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) { /* Note this ordering is used in conditional aces */ - return d1->length - d2->length; + return NUMERIC_CMP(d1->length, d2->length); } return ret; } -- 2.34.1 From ef5cc065373a8bf52dc7cbbe163c4b3f0de69ed3 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 afddbe40ef6..eb84392dd3d 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 87d838740d7bdd826c348fb9588054c451aaf257 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 eb84392dd3d..c334d70747a 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 59b2576021bf1eb894c67579fcb86a9710a6bc02 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 (cherry picked from commit cb94202c1cf990e871ee2e8e43c577a0e4b9ee6f) --- 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 eaece2a55f5..82816071a7a 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,9 +72,9 @@ 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; - + 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; -- 2.34.1 From ce21a1e1f68fb6f5dee18477310afebbcaaeaa0a 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 82816071a7a..21012b70884 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 a98e04d6deb3b7a56502bfb92819e565d6b873ec 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 c5809d364950c52613588863c482622391d713da 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 841c764031f..f8cdb514801 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 1390855c0072944c29608e234931315de2ce1c50 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 ccbc74c671c..1cbd7d94593 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 933fd4251ff520cdc7af04f3e4ea800cd9eb0110 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 54b64ab5c5ba85abc191f628a4a226ab08d05279 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 1f44bae94c78917e5d61f4b186326d4bc0e9a3db 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 f6e115fb9994f46a53ad4b76c9738d4ccfb1c7d8 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 4ff00013e8395a968b0a824bb3ae9dac6994f321 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 e0591ac076b..46bf79b839b 100644 --- a/source3/utils/smbcacls.c +++ b/source3/utils/smbcacls.c @@ -510,22 +510,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 b4ae2f52aa02f8fa9f880b6e2a990a594d84c759 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 a6481e25481..417572954c8 100644 --- a/source3/utils/sharesec.c +++ b/source3/utils/sharesec.c @@ -120,19 +120,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 9a57ac734a9fff4da1a064dcfb7d2aad07c0a795 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 dcb2f9e74a7..a9023419376 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 508ff55b786fa70cb29dfedc78ba5b813db80619 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 c2777afd205ca97b400e678c383d587f95c7874e 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 fa782b79f892e3b0598ce6819437f0dc1d9ad9a7 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 12ffd27c4f96cac00a6d4416e9f2f8bc665a3a45 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 fe853fc1f8b6cfa5dd0f736d44c098992a3fd798 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 8840ce09550326de4b8dad89c06c04351bd26860 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 4668a0c5e62447396a094165d440ddab30be76b5 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 aae2ca17337..ecc53d015f2 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 b61991524e507fa9aa3768d3e5f1ff632e14af67 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 f253215c8f438201c78eade454df6732dfdd5458 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 0724dd00af5..ed16278b9fc 100644 --- a/source3/rpc_server/wkssvc/srv_wkssvc_nt.c +++ b/source3/rpc_server/wkssvc/srv_wkssvc_nt.c @@ -50,7 +50,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 3aa4473bcca61584ea4f7702b6d1bf7e6722ad4b 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 072188a6752..bcf98bd5968 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 fd99366e90573c32d0985355bb28ba78cca8078f 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 4908b0f55e2..2426ce2b52a 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 022b31162bf2e8a1a3a01dbc03a31689b7d1de8b 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 232b8003973..2297dd9ded9 100644 --- a/source3/libsmb/nmblib.c +++ b/source3/libsmb/nmblib.c @@ -1235,8 +1235,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 1331486def6ac2956e0d7575aadb533adba2c9e1 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 e6d35fc00fc..6ffca196861 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 0a2adff870e57ca4f3693e8377580437a5f2bb30 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 f8cdb514801..66a7785a9f4 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 ca9379ec8e9e8583176e98778f373d9de8dae70a 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 c30fd6358c8..8873ee6e4e6 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 cf51d9ed45c2942101b59f2c64bc421caa635051 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 8873ee6e4e6..04b942a015b 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 346218f799fc018e5ab3d49f18760c954eabe8ab 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 04b942a015b..77cc1003a16 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 c81a20b92094a2cbe6fb247496ea637d3369dcc8 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 77cc1003a16..aebcd282272 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 9e5614e523abc7a2682554a9ba82de40480d18db 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 aebcd282272..1f39c5da36b 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 e7e28cc7515aa0f8a6108a3346bbc7d9c0f4b9e8 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 1f39c5da36b..4f9d67ec6a6 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 56e342653d189b0860b73139a1a6aef74643c30e 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 a5f3e42176ddd9f69b3d81bee2a87acc61454a34 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 ec41f930960d58ff476e784584d35ee7cd173a16 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 7aec0063c96..6239f569db7 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 4f8ae58c65c28f091f56d6ae42398f26cb26a405 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 6239f569db7..27906790e2c 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 1f16067a5b2bfb0203305db2575986e1343b2825 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 509eef6104393bde38b9c1f86c70da8d099fdc39 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 642aeb5a6d07d52981a82510efe1354e40a0fee0 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 46bf79b839b..5df71585c3b 100644 --- a/source3/utils/smbcacls.c +++ b/source3/utils/smbcacls.c @@ -510,7 +510,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