From f4dabc7a1244c7731572a3e2b91b94100fb6f2ae Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 11:06:00 +1300 Subject: [PATCH 01/64] 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 0bf801b640d0bea7facf265a969f4a1b61d9cca4 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 28 Mar 2024 12:57:54 +1300 Subject: [PATCH 02/64] 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 a2688a6bebee311044063030a3bc54fe8b91c160 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:43:27 +1300 Subject: [PATCH 03/64] 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 58d79dc0b938000f17e01da2caaca7c0a0e8ac0d Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 17:53:39 +1300 Subject: [PATCH 04/64] ldb: add NUMERIC_CMP macro to ldb.h In other places we tend to include tsort.h, which also has TYPESAFE_QSORT. ldb.h already has TYPESAFE_QSORT, so it might as well have NUMERIC_CMP. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit de1b94f79ea8694ecdddab4b455d539caa7e77e2) --- lib/ldb/include/ldb.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/ldb/include/ldb.h b/lib/ldb/include/ldb.h index 8bebd9fce1b..c495e7f1750 100644 --- a/lib/ldb/include/ldb.h +++ b/lib/ldb/include/ldb.h @@ -2326,6 +2326,22 @@ do { \ } while (0) #endif +#ifndef NUMERIC_CMP +/* + * NUMERIC_CMP is a safe replacement for `a - b` in comparison + * functions. It will work on integers, pointers, and floats. + * + * Rather than + * + * return a - b; + * + * use + * + * return NUMERIC_CMP(a, b); + */ +#define NUMERIC_CMP(a, b) (((a) > (b)) - ((a) < (b))) +#endif + /** -- 2.34.1 From 531162723dadad8d5fae4feffea00a7b7e11f6b0 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:50:47 +1300 Subject: [PATCH 05/64] 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 21601cbb1826a2e6cff9425eedae6e2bad29e99f Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:51:04 +1300 Subject: [PATCH 06/64] 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 9679a5d915493dfcfc0f098cd5d93e3ed7d0b93e Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:52:50 +1300 Subject: [PATCH 07/64] s4:ntvfs: use NUMERIC_CMP in stream_name_cmp BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit a6d76d6ee9f7cfcabe2c20b872b8b1cb598928a6) --- source4/ntvfs/posix/pvfs_streams.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source4/ntvfs/posix/pvfs_streams.c b/source4/ntvfs/posix/pvfs_streams.c index cacd8c19952..ae1ec113e8a 100644 --- a/source4/ntvfs/posix/pvfs_streams.c +++ b/source4/ntvfs/posix/pvfs_streams.c @@ -22,6 +22,7 @@ #include "includes.h" #include "vfs_posix.h" #include "librpc/gen_ndr/xattr.h" +#include "lib/util/tsort.h" /* normalise a stream name, removing a :$DATA suffix if there is one @@ -51,7 +52,7 @@ static int stream_name_cmp(const char *name1, const char *name2) l1 = c1?(c1 - name1):strlen(name1); l2 = c2?(c2 - name2):strlen(name2); if (l1 != l2) { - return l1 - l2; + return NUMERIC_CMP(l1, l2); } ret = strncasecmp_m(name1, name2, l1); if (ret != 0) { -- 2.34.1 From 4e488875038b34d339027dd2f8afcf21e0e57024 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:55:27 +1300 Subject: [PATCH 08/64] s4:dsdb:mod:operational: use NUMERIC_CMP in pso_compare prec_{1,2} are uint32_t, and if one is not set we are defaulting to 0xffffffff (a.k.a UINT32_MAX), so an overflow when cast to int seems extremely likely. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 623adcf4aae00ac06e82d98a75ce4644890501e6) --- source4/dsdb/samdb/ldb_modules/operational.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/dsdb/samdb/ldb_modules/operational.c b/source4/dsdb/samdb/ldb_modules/operational.c index 8821765a703..8c8bfa89dde 100644 --- a/source4/dsdb/samdb/ldb_modules/operational.c +++ b/source4/dsdb/samdb/ldb_modules/operational.c @@ -1073,7 +1073,7 @@ static int pso_compare(struct ldb_message **m1, struct ldb_message **m2) return ndr_guid_compare(&guid1, &guid2); } else { - return prec1 - prec2; + return NUMERIC_CMP(prec1, prec2); } } -- 2.34.1 From 5e8af1a052247dbf270018cb1f65cf9fcf7d3fbb Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:55:54 +1300 Subject: [PATCH 09/64] s4: use numeric_cmp in dns_common_sort_zones() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit ee4ebcccd7d9d89dda59615b3653df2632fb1a5d) --- source4/dns_server/dnsserver_common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/dns_server/dnsserver_common.c b/source4/dns_server/dnsserver_common.c index 0481b0715c7..0bbba7ff6de 100644 --- a/source4/dns_server/dnsserver_common.c +++ b/source4/dns_server/dnsserver_common.c @@ -1402,7 +1402,7 @@ static int dns_common_sort_zones(struct ldb_message **m1, struct ldb_message **m /* If the string lengths are not equal just sort by length */ if (l1 != l2) { /* If m1 is the larger zone name, return it first */ - return l2 - l1; + return NUMERIC_CMP(l2, l1); } /*TODO: We need to compare DNs here, we want the DomainDNSZones first */ -- 2.34.1 From c9c8a6401ba8d06313cda0545185e749bf28ce65 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 15:47:10 +1300 Subject: [PATCH 10/64] 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 4b99fd0fc80009de253a8b64a8ccd28c2794aff5 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 5 Apr 2024 13:14:38 +1300 Subject: [PATCH 11/64] 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 634d54371c3f1898fa72811d4a86fd341bfe7345 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 5 Apr 2024 14:43:42 +1300 Subject: [PATCH 12/64] 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 393d7290c2d42f74913dc910e75e0c65bd90b2dc Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 5 Apr 2024 14:46:48 +1300 Subject: [PATCH 13/64] 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 594552ef473960f9f8d0b38ac06e397422320cf8 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 15:49:55 +1300 Subject: [PATCH 14/64] util:charset:util_str: use NUMERIC_CMP in strcasecmp_m_handle BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit f788a399996a73b2aa206ec2b15f5943b06660e0) --- lib/util/charset/util_str.c | 5 +++-- selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard diff --git a/lib/util/charset/util_str.c b/lib/util/charset/util_str.c index 5d415f097e4..04d69dfb428 100644 --- a/lib/util/charset/util_str.c +++ b/lib/util/charset/util_str.c @@ -26,6 +26,7 @@ #include "system/locale.h" #include "charset.h" #include "lib/util/fault.h" +#include "lib/util/tsort.h" #ifdef strcasecmp #undef strcasecmp @@ -79,10 +80,10 @@ _PUBLIC_ int strcasecmp_m_handle(struct smb_iconv_handle *iconv_handle, continue; } - return l1 - l2; + return NUMERIC_CMP(l1, l2); } - return *s1 - *s2; + return NUMERIC_CMP(*s1, *s2); } /** diff --git a/selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard b/selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard new file mode 100644 index 00000000000..fe0d14e83e2 --- /dev/null +++ b/selftest/knownfail.d/test_ms_fn_match_protocol_no_wildcard @@ -0,0 +1 @@ +^samba.unittests.ms_fnmatch.test_ms_fn_match_protocol_no_wildcard -- 2.34.1 From 987c6de75ba269fc1b68a4512d3c98215eccef94 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 17:23:15 +1300 Subject: [PATCH 15/64] 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 18bf7d3f91fb0e3dca9b8831a87c1dbf99251b54 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 15:53:29 +1300 Subject: [PATCH 16/64] util:charset:codepoints: condepoint_cmpi uses NUMERIC_CMP() If these are truly unicode codepoints (< ~2m) there is no overflow, but the type is defined as uint32_t. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 675fdeee3d6570fdf5a055890dc3386a8db5fd88) --- lib/util/charset/codepoints.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/util/charset/codepoints.c b/lib/util/charset/codepoints.c index c39ed4a8436..b5220763d40 100644 --- a/lib/util/charset/codepoints.c +++ b/lib/util/charset/codepoints.c @@ -26,6 +26,7 @@ #include "dynconfig/dynconfig.h" #include "lib/util/debug.h" #include "lib/util/byteorder.h" +#include "lib/util/tsort.h" #ifdef strcasecmp #undef strcasecmp @@ -16483,7 +16484,7 @@ _PUBLIC_ int codepoint_cmpi(codepoint_t c1, codepoint_t c2) toupper_m(c1) == toupper_m(c2)) { return 0; } - return c1 - c2; + return NUMERIC_CMP(c1, c2); } -- 2.34.1 From 457975ba80065d31040de39bd86979f91b8373ce Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:56:16 +1300 Subject: [PATCH 17/64] util:charset:codepoints: codepoint_cmpi warning about non-transitivity BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit f07ae6990702f8806c0c815454b80a5596b7219a) --- lib/util/charset/codepoints.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/util/charset/codepoints.c b/lib/util/charset/codepoints.c index b5220763d40..cc263745b6d 100644 --- a/lib/util/charset/codepoints.c +++ b/lib/util/charset/codepoints.c @@ -16480,6 +16480,18 @@ _PUBLIC_ bool isupper_m(codepoint_t val) */ _PUBLIC_ int codepoint_cmpi(codepoint_t c1, codepoint_t c2) { + /* + * FIXME: this is unsuitable for use in a sort, as the + * comparison is intransitive. + * + * The problem is toupper_m() is only called on equality case, + * which has strange effects. + * + * Consider {'a', 'A', 'B'}. + * 'a' == 'A' + * 'a' > 'B' (lowercase letters come after upper) + * 'A' < 'B' + */ if (c1 == c2 || toupper_m(c1) == toupper_m(c2)) { return 0; -- 2.34.1 From d572ba098baffed2aa23d19cacf89aeb67060542 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 16:10:38 +1300 Subject: [PATCH 18/64] s3:libsmb:namequery: note intransitivity in addr_compare() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 4a9d274d43b1adac113419c649bbf530d180229d) --- source3/libsmb/namequery.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/source3/libsmb/namequery.c b/source3/libsmb/namequery.c index e6c0c7d2a09..01bac929776 100644 --- a/source3/libsmb/namequery.c +++ b/source3/libsmb/namequery.c @@ -1082,8 +1082,15 @@ bool name_status_find(const char *q_name, } /* - comparison function used by sort_addr_list -*/ + * comparison function used by sort_addr_list + * + * This comparison is intransitive in sort if a socket has an invalid + * family (i.e., not IPv4 or IPv6), or an interface doesn't support + * the family. Say we have sockaddrs with IP versions {4,5,6}, of + * which 5 is invalid. By this function, 4 == 5 and 6 == 5, but 4 != + * 6. This is of course a consequence of cmp() being unable to + * communicate error. + */ static int addr_compare(const struct sockaddr_storage *ss1, const struct sockaddr_storage *ss2) -- 2.34.1 From c370b72705877d89534eabdff2e6b47c557d681d Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 16:13:07 +1300 Subject: [PATCH 19/64] s3:libsmb:namequery: use NUMERIC_CMP in addr_compare This one was OK, as the numbers are tightly bound, but there is no real reason not to do it safely. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 3414a894ad6640fa8e282d650b1cc5319991545f) --- source3/libsmb/namequery.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source3/libsmb/namequery.c b/source3/libsmb/namequery.c index 01bac929776..8ae4004a98c 100644 --- a/source3/libsmb/namequery.c +++ b/source3/libsmb/namequery.c @@ -34,6 +34,7 @@ #include "lib/gencache.h" #include "librpc/gen_ndr/dns.h" #include "lib/util/util_net.h" +#include "lib/util/tsort.h" #include "lib/util/string_wrappers.h" /* nmbd.c sets this to True. */ @@ -1178,7 +1179,7 @@ static int addr_compare(const struct sockaddr_storage *ss1, max_bits2 += 128; } } - return max_bits2 - max_bits1; + return NUMERIC_CMP(max_bits2, max_bits1); } /* -- 2.34.1 From ae63c237e48a39fc98decad85e321835ad02d003 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 5 Apr 2024 14:22:11 +1300 Subject: [PATCH 20/64] lib/torture: add assert_int_{less,greater} macros In some situations, like comparison functions for qsort, we don't care about the actual value, just whethger it was greater or less than zero. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 6159b098cf35a8043682bfd4c4ea17ef0da6e8ee) --- lib/torture/torture.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/lib/torture/torture.h b/lib/torture/torture.h index 6818084ea96..3509911c8ca 100644 --- a/lib/torture/torture.h +++ b/lib/torture/torture.h @@ -525,6 +525,26 @@ static inline void torture_dump_data_str_cb(const char *buf, void *private_data) } \ } while(0) +#define torture_assert_int_less(torture_ctx,got,limit,cmt)\ + do { int __got = (got), __limit = (limit); \ + if (__got >= __limit) { \ + torture_result(torture_ctx, TORTURE_FAIL, \ + __location__": "#got" was %d (0x%X), expected < %d (0x%X): %s", \ + __got, __got, __limit, __limit, cmt); \ + return false; \ + } \ + } while(0) + +#define torture_assert_int_greater(torture_ctx,got,limit,cmt)\ + do { int __got = (got), __limit = (limit); \ + if (__got <= __limit) { \ + torture_result(torture_ctx, TORTURE_FAIL, \ + __location__": "#got" was %d (0x%X), expected > %d (0x%X): %s", \ + __got, __got, __limit, __limit, cmt); \ + return false; \ + } \ + } while(0) + #define torture_assert_int_equal_goto(torture_ctx,got,expected,ret,label,cmt)\ do { int __got = (got), __expected = (expected); \ if (__got != __expected) { \ -- 2.34.1 From 9bad3e229bc77593c0e8aebb55573bb3b31bc5eb Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 16:16:44 +1300 Subject: [PATCH 21/64] util: charset:util_str: use NUMERIC_CMP in strncasecmp_m_handle BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 997b72d79e651ddbc20e67006ae176229528dc6f) --- lib/util/charset/util_str.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/util/charset/util_str.c b/lib/util/charset/util_str.c index 04d69dfb428..c4db52160f4 100644 --- a/lib/util/charset/util_str.c +++ b/lib/util/charset/util_str.c @@ -157,14 +157,14 @@ _PUBLIC_ int strncasecmp_m_handle(struct smb_iconv_handle *iconv_handle, continue; } - return l1 - l2; + return NUMERIC_CMP(l1, l2); } if (n == 0) { return 0; } - return *s1 - *s2; + return NUMERIC_CMP(*s1, *s2); } /** -- 2.34.1 From 9f26a7eab826e379303a30c43b769d71c7808211 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 17:32:48 +1300 Subject: [PATCH 22/64] 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 ece71bb5170fcf828304f55c7d267c127bf9449f Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 17:43:03 +1300 Subject: [PATCH 23/64] 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 7565038132321b8db28f43a4c3af1422a2aee11c Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 11:07:06 +1300 Subject: [PATCH 24/64] util:datablob: avoid non-transitive comparison in data_blob_cmp() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (backported from commit e1519c3667841ce27b15983eae378799ef9936f7) [dbagnall@samba.org: changed in master for conditional ACEs] --- lib/util/data_blob.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/util/data_blob.c b/lib/util/data_blob.c index 677f7c19211..687b6c8d3d5 100644 --- a/lib/util/data_blob.c +++ b/lib/util/data_blob.c @@ -22,6 +22,7 @@ #include "attr.h" #include "data_blob.h" #include "lib/util/samba_util.h" +#include "lib/util/tsort.h" const DATA_BLOB data_blob_null = { NULL, 0 }; @@ -121,11 +122,11 @@ _PUBLIC_ int data_blob_cmp(const DATA_BLOB *d1, const DATA_BLOB *d2) return 1; } if (d1->data == d2->data) { - return d1->length - d2->length; + return NUMERIC_CMP(d1->length, d2->length); } ret = memcmp(d1->data, d2->data, MIN(d1->length, d2->length)); if (ret == 0) { - return d1->length - d2->length; + return NUMERIC_CMP(d1->length, d2->length); } return ret; } -- 2.34.1 From 5aeb8935cdb3e4d0330ed8861b28520a1a3332d9 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 11:22:58 +1300 Subject: [PATCH 25/64] ldb: avoid non-transitive comparison in ldb_val_cmp() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 5c36bc82415b246fccec9eae693da82b7aa45b81) --- lib/ldb/common/ldb_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldb/common/ldb_msg.c b/lib/ldb/common/ldb_msg.c index 3242447b544..5c5f6a7c2b4 100644 --- a/lib/ldb/common/ldb_msg.c +++ b/lib/ldb/common/ldb_msg.c @@ -93,7 +93,7 @@ struct ldb_val *ldb_msg_find_val(const struct ldb_message_element *el, static int ldb_val_cmp(const struct ldb_val *v1, const struct ldb_val *v2) { if (v1->length != v2->length) { - return v1->length - v2->length; + return NUMERIC_CMP(v1->length, v2->length); } return memcmp(v1->data, v2->data, v1->length); } -- 2.34.1 From 22b1ec447c1002ce09103eddd1230a4e15b9e70d Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 11:26:25 +1300 Subject: [PATCH 26/64] ldb: reduce non-transitive comparisons in ldb_msg_element_compare() We can still have inconsistent comparisons, because two elements with the same number of values will always return -1 if they are unequal, which means they will sort differently depending on the order in which they are compared. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 21a071e4864dd739840c2ad4adb0c71ec33f8427) --- lib/ldb/common/ldb_msg.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/ldb/common/ldb_msg.c b/lib/ldb/common/ldb_msg.c index 5c5f6a7c2b4..e8164474882 100644 --- a/lib/ldb/common/ldb_msg.c +++ b/lib/ldb/common/ldb_msg.c @@ -749,9 +749,16 @@ int ldb_msg_element_compare(struct ldb_message_element *el1, unsigned int i; if (el1->num_values != el2->num_values) { - return el1->num_values - el2->num_values; + return NUMERIC_CMP(el1->num_values, el2->num_values); } - + /* + * Note this is an inconsistent comparison, unsuitable for + * sorting. If A has values {a, b} and B has values {b, c}, + * then + * + * ldb_msg_element_compare(A, B) returns -1, meaning A < B + * ldb_msg_element_compare(B, A) returns -1, meaning B < A + */ for (i=0;inum_values;i++) { if (!ldb_msg_find_val(el2, &el1->values[i])) { return -1; -- 2.34.1 From 7d8a61cde637a2cb5244e29c04252aaa3916020d Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 13:43:47 +1300 Subject: [PATCH 27/64] libcli/security: use NUMERIC_CMP in dom_sid_compare() sid->num_auths is always small (int8 < 16), so this is cosmetic only. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (backported from commit cb94202c1cf990e871ee2e8e43c577a0e4b9ee6f) [dbagnall@samba.org: file changed in master] --- libcli/security/dom_sid.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/libcli/security/dom_sid.c b/libcli/security/dom_sid.c index 9a91760ff62..38f98f9f906 100644 --- a/libcli/security/dom_sid.c +++ b/libcli/security/dom_sid.c @@ -28,6 +28,7 @@ #include "librpc/gen_ndr/security.h" #include "dom_sid.h" #include "lib/util/smb_strtox.h" +#include "lib/util/tsort.h" /***************************************************************** Compare the auth portion of two sids. @@ -71,12 +72,17 @@ int dom_sid_compare(const struct dom_sid *sid1, const struct dom_sid *sid2) return 1; /* Compare most likely different rids, first: i.e start at end */ - if (sid1->num_auths != sid2->num_auths) - return sid1->num_auths - sid2->num_auths; - - for (i = sid1->num_auths-1; i >= 0; --i) - if (sid1->sub_auths[i] != sid2->sub_auths[i]) - return sid1->sub_auths[i] - sid2->sub_auths[i]; + if (sid1->num_auths != sid2->num_auths) { + return NUMERIC_CMP(sid1->num_auths, sid2->num_auths); + } + for (i = sid1->num_auths-1; i >= 0; --i) { + if (sid1->sub_auths[i] < sid2->sub_auths[i]) { + return -1; + } + if (sid1->sub_auths[i] > sid2->sub_auths[i]) { + return 1; + } + } return dom_sid_compare_auth(sid1, sid2); } -- 2.34.1 From 6ce1612f14a6d8022adab3d2a5ad22d7d07e889a Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 13:53:58 +1300 Subject: [PATCH 28/64] libcli/security: use NUMERIC_CMP in dom_sid_compare_auth() These numbers are all 8 bit, so overflow is unlikely. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 4641a97151783c2ae825582e91b4676d66dcb713) --- libcli/security/dom_sid.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libcli/security/dom_sid.c b/libcli/security/dom_sid.c index 38f98f9f906..737f4bb3a35 100644 --- a/libcli/security/dom_sid.c +++ b/libcli/security/dom_sid.c @@ -47,11 +47,12 @@ int dom_sid_compare_auth(const struct dom_sid *sid1, return 1; if (sid1->sid_rev_num != sid2->sid_rev_num) - return sid1->sid_rev_num - sid2->sid_rev_num; + return NUMERIC_CMP(sid1->sid_rev_num, sid2->sid_rev_num); for (i = 0; i < 6; i++) - if (sid1->id_auth[i] != sid2->id_auth[i]) - return sid1->id_auth[i] - sid2->id_auth[i]; + if (sid1->id_auth[i] != sid2->id_auth[i]) { + return NUMERIC_CMP(sid1->id_auth[i], sid2->id_auth[i]); + } return 0; } -- 2.34.1 From b6cb2b7f1c01bcc809e2df074c6988c877aa5ede Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:01:24 +1300 Subject: [PATCH 29/64] 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 658c6dc44ae47e944b64e0a5fdec9a50f279d67f Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:10:45 +1300 Subject: [PATCH 30/64] s4:rpc_server: compare_SamEntry() uses NUMERIC_CMP() SamEntry.idx is uint32_t. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit ed3ab87bdb0f6c6a9ea6323ed240fe267220b759) --- source4/rpc_server/samr/dcesrv_samr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/rpc_server/samr/dcesrv_samr.c b/source4/rpc_server/samr/dcesrv_samr.c index 5570359728f..ba5be53d8c9 100644 --- a/source4/rpc_server/samr/dcesrv_samr.c +++ b/source4/rpc_server/samr/dcesrv_samr.c @@ -1166,7 +1166,7 @@ static NTSTATUS dcesrv_samr_CreateDomainGroup(struct dcesrv_call_state *dce_call */ static int compare_SamEntry(struct samr_SamEntry *e1, struct samr_SamEntry *e2) { - return e1->idx - e2->idx; + return NUMERIC_CMP(e1->idx, e2->idx); } static int compare_msgRid(struct ldb_message **m1, struct ldb_message **m2) { -- 2.34.1 From 3f23186033d81931ee298380d976e79ddcec3234 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:22:24 +1300 Subject: [PATCH 31/64] s4:dns_server: use NUMERIC_CMP in rec_cmp() dnsp_DnssrvRpcRecord.dwTimeStamp is uint32_t, making overflow possible. dnsp_DnssrvRpcRecord.wType is an enum, which has the size of an int, though it may be hard to set it to overflowing values. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 42ead213484840121ce6bc0db22941ea0a019105) --- source4/dns_server/dnsserver_common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source4/dns_server/dnsserver_common.c b/source4/dns_server/dnsserver_common.c index 0bbba7ff6de..79468dbb8fe 100644 --- a/source4/dns_server/dnsserver_common.c +++ b/source4/dns_server/dnsserver_common.c @@ -642,7 +642,7 @@ static int rec_cmp(const struct dnsp_DnssrvRpcRecord *r1, * The records are sorted with higher types first, * which puts tombstones (type 0) last. */ - return r2->wType - r1->wType; + return NUMERIC_CMP(r2->wType, r1->wType); } /* * Then we need to sort from the oldest to newest timestamp. @@ -650,7 +650,7 @@ static int rec_cmp(const struct dnsp_DnssrvRpcRecord *r1, * Note that dwTimeStamp == 0 (never expiring) records come first, * then the ones whose expiry is soonest. */ - return r1->dwTimeStamp - r2->dwTimeStamp; + return NUMERIC_CMP(r1->dwTimeStamp, r2->dwTimeStamp); } /* -- 2.34.1 From bb37c359447e478e6b2f37395de298baa39c447c Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:54:09 +1300 Subject: [PATCH 32/64] 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 e6d11c55f0c8b48038338c0c8bf03c4f58bb52b9 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:16:21 +1300 Subject: [PATCH 33/64] 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 634c3691ac03be24268bf5c2101f6387c800569f Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:53:32 +1300 Subject: [PATCH 34/64] 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 5592539deb8cbd267dee2e8d89cd5d4cf45ce30a Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:25:54 +1300 Subject: [PATCH 35/64] 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 d790311fee2ab87319b3e05e6991bf4e6bae6c0e Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 3 Apr 2024 12:56:48 +1300 Subject: [PATCH 36/64] s3:smbcacls: use NUMERIC_CMP in ace_compare BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 31101a9fa1503be9d8137e42466f57d85136a156) --- source3/utils/smbcacls.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/source3/utils/smbcacls.c b/source3/utils/smbcacls.c index 59913f63b11..afebcd40b32 100644 --- a/source3/utils/smbcacls.c +++ b/source3/utils/smbcacls.c @@ -506,22 +506,23 @@ static int ace_compare(struct security_ace *ace1, struct security_ace *ace2) return -1; if ((ace1->flags & SEC_ACE_FLAG_INHERITED_ACE) && (ace2->flags & SEC_ACE_FLAG_INHERITED_ACE)) - return ace1 - ace2; - - if (ace1->type != ace2->type) - return ace2->type - ace1->type; + return NUMERIC_CMP(ace1, ace2); + if (ace1->type != ace2->type) { + /* note the reverse order */ + return NUMERIC_CMP(ace2->type, ace1->type); + } if (dom_sid_compare(&ace1->trustee, &ace2->trustee)) return dom_sid_compare(&ace1->trustee, &ace2->trustee); if (ace1->flags != ace2->flags) - return ace1->flags - ace2->flags; + return NUMERIC_CMP(ace1->flags, ace2->flags); if (ace1->access_mask != ace2->access_mask) - return ace1->access_mask - ace2->access_mask; + return NUMERIC_CMP(ace1->access_mask, ace2->access_mask); if (ace1->size != ace2->size) - return ace1->size - ace2->size; + return NUMERIC_CMP(ace1->size, ace2->size); return memcmp(ace1, ace2, sizeof(struct security_ace)); } -- 2.34.1 From 2880cd57f95006b4eefbee5d248a26cb73c26a7e Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:08:02 +1300 Subject: [PATCH 37/64] s3:util:sharesec ace_compare() uses NUMERIC_CMP() ace->access_mask is uint32_t, so can overflow a signed int. This would be easy to trigger, as it is a flags field rather than an allocation count. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit e35d54fd4d381df67ab9b4f8390e2109b2142678) --- source3/utils/sharesec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source3/utils/sharesec.c b/source3/utils/sharesec.c index 9b8064de702..8f6117209f4 100644 --- a/source3/utils/sharesec.c +++ b/source3/utils/sharesec.c @@ -119,19 +119,19 @@ static int ace_compare(struct security_ace *ace1, struct security_ace *ace2) return 0; if (ace1->type != ace2->type) - return ace2->type - ace1->type; + return NUMERIC_CMP(ace2->type, ace1->type); if (dom_sid_compare(&ace1->trustee, &ace2->trustee)) return dom_sid_compare(&ace1->trustee, &ace2->trustee); if (ace1->flags != ace2->flags) - return ace1->flags - ace2->flags; + return NUMERIC_CMP(ace1->flags, ace2->flags); if (ace1->access_mask != ace2->access_mask) - return ace1->access_mask - ace2->access_mask; + return NUMERIC_CMP(ace1->access_mask, ace2->access_mask); if (ace1->size != ace2->size) - return ace1->size - ace2->size; + return NUMERIC_CMP(ace1->size, ace2->size); return memcmp(ace1, ace2, sizeof(struct security_ace)); } -- 2.34.1 From 4e5247124212b2443a51d0682b2c77cb7d64bbfb Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 4 Apr 2024 14:33:47 +1300 Subject: [PATCH 38/64] s3:libsmb_xattr: ace_compare() uses NUMERIC_CMP() the access_mask is the easiest to overflow with subtraction -- other fields are 8 or 16 bit. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett Autobuild-User(master): Andrew Bartlett Autobuild-Date(master): Wed Apr 10 23:58:12 UTC 2024 on atb-devel-224 (cherry picked from commit 81598b42455d6758941da532c668b6d4e969cc40) --- source3/libsmb/libsmb_xattr.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/source3/libsmb/libsmb_xattr.c b/source3/libsmb/libsmb_xattr.c index 1f820521193..67cffe14971 100644 --- a/source3/libsmb/libsmb_xattr.c +++ b/source3/libsmb/libsmb_xattr.c @@ -121,7 +121,13 @@ ace_compare(struct security_ace *ace1, */ if (ace1->type != ace2->type) { - return ace2->type - ace1->type; + /* + * ace2 and ace1 are reversed here, so that + * ACCESS_DENIED_ACE_TYPE (1) sorts before + * ACCESS_ALLOWED_ACE_TYPE (0), which is the order you + * usually want. + */ + return NUMERIC_CMP(ace2->type, ace1->type); } if (dom_sid_compare(&ace1->trustee, &ace2->trustee)) { @@ -129,15 +135,15 @@ ace_compare(struct security_ace *ace1, } if (ace1->flags != ace2->flags) { - return ace1->flags - ace2->flags; + return NUMERIC_CMP(ace1->flags, ace2->flags); } if (ace1->access_mask != ace2->access_mask) { - return ace1->access_mask - ace2->access_mask; + return NUMERIC_CMP(ace1->access_mask, ace2->access_mask); } if (ace1->size != ace2->size) { - return ace1->size - ace2->size; + return NUMERIC_CMP(ace1->size, ace2->size); } return memcmp(ace1, ace2, sizeof(struct security_ace)); -- 2.34.1 From 58081d115a29d6722efbc2249747af3617777750 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 14:54:34 +1200 Subject: [PATCH 39/64] 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 a3f762be01e65c3ee86d4d75e6e7306f2b450938 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 14:55:27 +1200 Subject: [PATCH 40/64] 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 08f73155f51f96bebe88c60a4b8ab32b7e0c5482 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 14:58:48 +1200 Subject: [PATCH 41/64] 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 895978f1518e136303bf8d83a9963f396ccfc0a3 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:04:43 +1200 Subject: [PATCH 42/64] 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 21d3cea5c5a83a0b5b65ea34fd03aee643856455 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:07:20 +1200 Subject: [PATCH 43/64] 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 2d184aa2285746a7c4e75ac50e14c93d7fd031ac Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:12:56 +1200 Subject: [PATCH 44/64] 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 2caea4901c9ae9efae1ea6cfd56138f19bd91e39 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:17:22 +1200 Subject: [PATCH 45/64] s3:mod:vfs_vxfs: use NUMERIC_CMP in vxfs_ace_cmp BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 386216d4a158d8bafb0879a0a753da096a939b93) --- source3/modules/vfs_vxfs.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source3/modules/vfs_vxfs.c b/source3/modules/vfs_vxfs.c index c41d050e4ef..96db010226d 100644 --- a/source3/modules/vfs_vxfs.c +++ b/source3/modules/vfs_vxfs.c @@ -111,13 +111,13 @@ static int vxfs_ace_cmp(const void *ace1, const void *ace2) type_a1 = SVAL(ace1, 0); type_a2 = SVAL(ace2, 0); - ret = (type_a1 - type_a2); - if (!ret) { + ret = NUMERIC_CMP(type_a1, type_a2); + if (ret == 0) { /* Compare ID under type */ /* skip perm thus take offset as 4*/ id_a1 = IVAL(ace1, 4); id_a2 = IVAL(ace2, 4); - ret = id_a1 - id_a2; + ret = NUMERIC_CMP(id_a1, id_a2); } return ret; -- 2.34.1 From 9ffabf11c79864c4a9381441973fe8e8267c1c29 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:36:06 +1200 Subject: [PATCH 46/64] 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 aa4a095905604d48d1155a88e188303ae213bc2f Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:47:12 +1200 Subject: [PATCH 47/64] s3:rpc:wkssvc_nt: dom_user_cmp uses NUMERIC_CMP usr->login_time is time_t, which is often bigger than int. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 75682e397b9cf22d04a5d80252554c6b2e376793) --- source3/rpc_server/wkssvc/srv_wkssvc_nt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/rpc_server/wkssvc/srv_wkssvc_nt.c b/source3/rpc_server/wkssvc/srv_wkssvc_nt.c index 592e8668cd3..89cdeb6daa1 100644 --- a/source3/rpc_server/wkssvc/srv_wkssvc_nt.c +++ b/source3/rpc_server/wkssvc/srv_wkssvc_nt.c @@ -140,7 +140,7 @@ static int dom_user_cmp(const struct dom_usr *usr1, const struct dom_usr *usr2) /* Called from qsort to compare two domain users in a dom_usr_t array * for sorting by login time. Return >0 if usr1 login time was later * than usr2 login time, <0 if it was earlier */ - return (usr1->login_time - usr2->login_time); + return NUMERIC_CMP(usr1->login_time, usr2->login_time); } /******************************************************************* -- 2.34.1 From 31478f5f22b8fe500f8313a07892cc02e5b03662 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sun, 7 Apr 2024 15:54:02 +1200 Subject: [PATCH 48/64] gensec: sort_gensec uses NUMERIC_CMP BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit acaa1323d0337ae9339dfff9f856ea54725a86ac) --- auth/gensec/gensec_start.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/auth/gensec/gensec_start.c b/auth/gensec/gensec_start.c index b6979812ac0..4511674c8eb 100644 --- a/auth/gensec/gensec_start.c +++ b/auth/gensec/gensec_start.c @@ -1103,7 +1103,7 @@ _PUBLIC_ const struct gensec_critical_sizes *gensec_interface_version(void) } static int sort_gensec(const struct gensec_security_ops **gs1, const struct gensec_security_ops **gs2) { - return (*gs2)->priority - (*gs1)->priority; + return NUMERIC_CMP((*gs2)->priority, (*gs1)->priority); } int gensec_setting_int(struct gensec_settings *settings, const char *mechanism, const char *name, int default_value) -- 2.34.1 From df78d8646be60ea04e51962a4601157323fcf924 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Mon, 8 Apr 2024 17:06:57 +1200 Subject: [PATCH 49/64] lib/socket: rearrange iface_comp() to use NUMERIC_CMP We rearrange rather than just replacing the subtraction, because that would call ntohl() more than necessary, and I think the flow is a bit clearer this way. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 7ba6fcb93656e5e88e1d5bcd6002747aa64f0a3a) --- lib/socket/interfaces.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/socket/interfaces.c b/lib/socket/interfaces.c index d8724e6c669..76a6570696c 100644 --- a/lib/socket/interfaces.c +++ b/lib/socket/interfaces.c @@ -386,18 +386,18 @@ static int iface_comp(struct iface_struct *i1, struct iface_struct *i2) if (((struct sockaddr *)&i1->ip)->sa_family == AF_INET) { struct sockaddr_in *s1 = (struct sockaddr_in *)&i1->ip; struct sockaddr_in *s2 = (struct sockaddr_in *)&i2->ip; - - r = ntohl(s1->sin_addr.s_addr) - - ntohl(s2->sin_addr.s_addr); - if (r) { - return r; + uint32_t a1 = ntohl(s1->sin_addr.s_addr); + uint32_t a2 = ntohl(s2->sin_addr.s_addr); + r = NUMERIC_CMP(a1, a2); + if (r == 0) { + /* compare netmasks as a tiebreaker */ + s1 = (struct sockaddr_in *)&i1->netmask; + s2 = (struct sockaddr_in *)&i2->netmask; + a1 = ntohl(s1->sin_addr.s_addr); + a2 = ntohl(s2->sin_addr.s_addr); + r = NUMERIC_CMP(a1, a2); } - - s1 = (struct sockaddr_in *)&i1->netmask; - s2 = (struct sockaddr_in *)&i2->netmask; - - return ntohl(s1->sin_addr.s_addr) - - ntohl(s2->sin_addr.s_addr); + return r; } return 0; } -- 2.34.1 From a43f9649a4221418a7c1d09ca97938fd78e6df8c Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Mon, 8 Apr 2024 17:08:03 +1200 Subject: [PATCH 50/64] s3:libsmb:nmblib: use NUMERIC_CMP in status_compare BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 31c322874b8b65518cec945e05a42fd014e6390b) --- source3/libsmb/nmblib.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source3/libsmb/nmblib.c b/source3/libsmb/nmblib.c index c90e92ebb69..ea4c5b7b35a 100644 --- a/source3/libsmb/nmblib.c +++ b/source3/libsmb/nmblib.c @@ -1229,8 +1229,10 @@ static unsigned char sort_ip[4]; static int name_query_comp(unsigned char *p1, unsigned char *p2) { - return matching_len_bits(p2+2, sort_ip, 4) - - matching_len_bits(p1+2, sort_ip, 4); + int a = matching_len_bits(p1+2, sort_ip, 4); + int b = matching_len_bits(p2+2, sort_ip, 4); + /* reverse sort -- p2 derived value comes first */ + return NUMERIC_CMP(b, a); } /**************************************************************************** -- 2.34.1 From f0c2975ab5dbd762f53e48183cc2c00274a19428 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Mon, 8 Apr 2024 22:54:49 +1200 Subject: [PATCH 51/64] s4:rpcsrv:dnsserver: make dns_name_compare transitive with NULLs Returning 0 on `(name1 == NULL || name2 == NULL)` made NULL equal to everything, which confuses a sort (consider {A, B, NULL} where A > B, but A == NULL == B). The only caller is dnsserver_enumerate_records() which fails if it finds a NULL in the sorted list. We make the happen more quickly by sorting NULLs to the front. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 7be535315a5eed5d5b7eaea025ecf9f55e772e8e) --- source4/rpc_server/dnsserver/dnsdata.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/source4/rpc_server/dnsserver/dnsdata.c b/source4/rpc_server/dnsserver/dnsdata.c index 002d9e622cc..4b2c04a089e 100644 --- a/source4/rpc_server/dnsserver/dnsdata.c +++ b/source4/rpc_server/dnsserver/dnsdata.c @@ -1075,9 +1075,23 @@ int dns_name_compare(struct ldb_message * const *m1, struct ldb_message * const name1 = ldb_msg_find_attr_as_string(*m1, "name", NULL); name2 = ldb_msg_find_attr_as_string(*m2, "name", NULL); - if (name1 == NULL || name2 == NULL) { + /* + * We sort NULL names to the start of the list, because the only + * caller of this function, dnsserver_enumerate_records() will call + * dns_build_tree() with the sorted list, which will always return an + * error when it hits a NULL, so we might as well make that happen + * quickly. + */ + if (name1 == name2) { + /* this includes the both NULL case */ return 0; } + if (name1 == NULL) { + return -1; + } + if (name2 == NULL) { + return 1; + } /* Compare the last components of names. * If search_name is not NULL, compare the second last components of names */ -- 2.34.1 From 3c65ecbd07c1b74f65373f702bc0141aea564cf3 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Mon, 8 Apr 2024 22:55:50 +1200 Subject: [PATCH 52/64] s4:rpcsrv:samr: improve a comment in compare_msgRid BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 6229feab74a734190c302ee9b1cc36960669743d) --- source4/rpc_server/samr/dcesrv_samr.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/source4/rpc_server/samr/dcesrv_samr.c b/source4/rpc_server/samr/dcesrv_samr.c index ba5be53d8c9..8faa457e80f 100644 --- a/source4/rpc_server/samr/dcesrv_samr.c +++ b/source4/rpc_server/samr/dcesrv_samr.c @@ -1197,8 +1197,9 @@ static int compare_msgRid(struct ldb_message **m1, struct ldb_message **m2) { } /* - * Get and compare the rids, if we fail to extract a rid treat it as a - * missing SID and sort to the end of the list + * Get and compare the rids. If we fail to extract a rid (because + * there are no subauths) the msg goes to the end of the list, but + * before the NULL SIDs. */ status = dom_sid_split_rid(NULL, sid1, NULL, &rid1); if (!NT_STATUS_IS_OK(status)) { -- 2.34.1 From 1a9b875d6e5ba87a01f3d8353eec0b1d0423534d Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 10 Apr 2024 10:54:31 +1200 Subject: [PATCH 53/64] ldb-samba: ldif-handlers: make ldif_comparison_objectSid() accurate This function compares blobs that might be SID strings or might be SID structures. Until now, if they were both (seemingly) strings, they were compared as strings, otherwise if either was a string it was converted to a structure blob, then the blobs were compared. This had two big problems: 1. There is variety in the way a SID can be stringified. For example, "s-1-02-3" means the same SID as "S-1-2-3", but those wouldn't compare equal. 2. SID comparison was crazily non-transitive. Consider the three values a = "S-1-2-3-4-5", b = "S-1-9-1", c = SID("S-1-11-1"), where c is a struct and the others are string. then we had, a < b, because the 5th character '2' < '9'. a > c, because when converted to a structure, the number of sub-auths is the first varying byte. a has 3, c has 0. b < c, because after the sub-auth count comes the id_auth value (big-endian, which doesn't matter in this case). That made the function unreliable for sorting, AND for simple equality tests. Also it leaked. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 6722e80d1b3a252a1ed714be4a35185cd99971e3) --- lib/ldb-samba/ldif_handlers.c | 59 +++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 24 deletions(-) diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c index f77a268c1a8..52244971931 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -150,36 +150,47 @@ bool ldif_comparision_objectSid_isString(const struct ldb_val *v) /* compare two objectSids + + If the SIDs seem to be strings, they are converted to binary form. */ static int ldif_comparison_objectSid(struct ldb_context *ldb, void *mem_ctx, const struct ldb_val *v1, const struct ldb_val *v2) { - if (ldif_comparision_objectSid_isString(v1) && ldif_comparision_objectSid_isString(v2)) { - return ldb_comparison_binary(ldb, mem_ctx, v1, v2); - } else if (ldif_comparision_objectSid_isString(v1) - && !ldif_comparision_objectSid_isString(v2)) { - struct ldb_val v; - int ret; - if (ldif_read_objectSid(ldb, mem_ctx, v1, &v) != 0) { - /* Perhaps not a string after all */ - return ldb_comparison_binary(ldb, mem_ctx, v1, v2); + bool v1_is_string = ldif_comparision_objectSid_isString(v1); + bool v2_is_string = ldif_comparision_objectSid_isString(v2); + struct ldb_val parsed_1 = {}; + struct ldb_val parsed_2 = {}; + int ret; + /* + * If the ldb_vals look like SID strings (i.e. start with "S-" + * or "s-"), we try to parse them as such. If that fails, we + * assume they are binary SIDs, even though that's not really + * possible -- the first two bytes of a struct dom_sid are the + * version (1), and the number of sub-auths (<= 15), neither + * of which are close to 'S' or '-'. + */ + if (v1_is_string) { + int r = ldif_read_objectSid(ldb, mem_ctx, v1, &parsed_1); + if (r == 0) { + v1 = &parsed_1; } - ret = ldb_comparison_binary(ldb, mem_ctx, &v, v2); - talloc_free(v.data); - return ret; - } else if (!ldif_comparision_objectSid_isString(v1) - && ldif_comparision_objectSid_isString(v2)) { - struct ldb_val v; - int ret; - if (ldif_read_objectSid(ldb, mem_ctx, v2, &v) != 0) { - /* Perhaps not a string after all */ - return ldb_comparison_binary(ldb, mem_ctx, v1, v2); - } - ret = ldb_comparison_binary(ldb, mem_ctx, v1, &v); - talloc_free(v.data); - return ret; } - return ldb_comparison_binary(ldb, mem_ctx, v1, v2); + if (v2_is_string) { + int r = ldif_read_objectSid(ldb, mem_ctx, v2, &parsed_2); + if (r == 0) { + v2 = &parsed_2; + } + } + + ret = ldb_comparison_binary(ldb, mem_ctx, v1, v2); + + if (v1_is_string) { + TALLOC_FREE(parsed_1.data); + } + if (v2_is_string) { + TALLOC_FREE(parsed_2.data); + } + return ret; } /* -- 2.34.1 From 6b3d37151f0f65427a3391eeb4cdb9c20be40418 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Apr 2024 16:25:02 +1200 Subject: [PATCH 54/64] ldb-samba:ldif_handlers: dn_link_comparison semi-sorts deleted objects We were always returning -1 for a deleted object, which works for an equality test, but not a relative comparison. This sorts deleted DNs toward the end of the list -- except when both DNs are deleted. What should happen there is yet to be determined. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit db963b1674ede357d4edba578e0e0372dcb2f287) --- lib/ldb-samba/ldif_handlers.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c index 52244971931..896315de44d 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1159,13 +1159,15 @@ static int samba_ldb_dn_link_comparison(struct ldb_context *ldb, void *mem_ctx, struct ldb_dn *dn1 = NULL, *dn2 = NULL; int ret; + /* + * In a sort context, Deleted DNs get shifted to the end. + * They never match in an equality + */ if (dsdb_dn_is_deleted_val(v1)) { - /* If the DN is deleted, then we can't search for it */ - return -1; + return 1; } if (dsdb_dn_is_deleted_val(v2)) { - /* If the DN is deleted, then we can't search for it */ return -1; } -- 2.34.1 From ddd4f51297480ff684108b21ae6702db28c935b2 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Apr 2024 16:26:03 +1200 Subject: [PATCH 55/64] ldb-samba:ldif_handlers: dn_link_comparison semi-sorts invalid DNs these tend to go to the end of the sorted array. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 11d5a809325369b48d14023adf109e418bb1c7af) --- lib/ldb-samba/ldif_handlers.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c index 896315de44d..11fc242eba3 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1172,7 +1172,9 @@ static int samba_ldb_dn_link_comparison(struct ldb_context *ldb, void *mem_ctx, } dn1 = ldb_dn_from_ldb_val(mem_ctx, ldb, v1); - if ( ! ldb_dn_validate(dn1)) return -1; + if ( ! ldb_dn_validate(dn1)) { + return 1; + } dn2 = ldb_dn_from_ldb_val(mem_ctx, ldb, v2); if ( ! ldb_dn_validate(dn2)) { -- 2.34.1 From a9d8eecdd98fd5b517ef062f83f0250100f31297 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Apr 2024 16:53:03 +1200 Subject: [PATCH 56/64] ldb-samba:ldif_handlers: dn_link_comparison correctly sorts deleted objects This changes the behaviour of the DN syntax .comparison_fn when being used in a search, if the search key is a deleted DN. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 70356592563bf758dbe509413445b77bb0d7da14) --- lib/ldb-samba/ldif_handlers.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c index 11fc242eba3..df5a23c4458 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1164,10 +1164,17 @@ static int samba_ldb_dn_link_comparison(struct ldb_context *ldb, void *mem_ctx, * They never match in an equality */ if (dsdb_dn_is_deleted_val(v1)) { - return 1; - } - - if (dsdb_dn_is_deleted_val(v2)) { + if (! dsdb_dn_is_deleted_val(v2)) { + return 1; + } + /* + * They are both deleted! + * + * The soundest thing to do at this point is carry on + * and compare the DNs normally. This matches the + * behaviour of samba_dn_extended_match() below. + */ + } else if (dsdb_dn_is_deleted_val(v2)) { return -1; } -- 2.34.1 From 84bea4ba6606c28161fd65db3207229084d84a33 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Apr 2024 16:59:50 +1200 Subject: [PATCH 57/64] ldb-samba:ldif_handlers: dn_link_comparison leaks less dn1 and dn2 can be invalid but still occupying memory. (ldb_dn_validate(dn2) does contain a NULL check, but a lot more besides). BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 341b8fb60e291ad598fafd7a09a75e9b249de07f) --- lib/ldb-samba/ldif_handlers.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c index df5a23c4458..90973cbf3c3 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1180,12 +1180,14 @@ static int samba_ldb_dn_link_comparison(struct ldb_context *ldb, void *mem_ctx, dn1 = ldb_dn_from_ldb_val(mem_ctx, ldb, v1); if ( ! ldb_dn_validate(dn1)) { + TALLOC_FREE(dn1); return 1; } dn2 = ldb_dn_from_ldb_val(mem_ctx, ldb, v2); if ( ! ldb_dn_validate(dn2)) { - talloc_free(dn1); + TALLOC_FREE(dn1); + TALLOC_FREE(dn2); return -1; } -- 2.34.1 From d7d647551d4355b7c9f106cd34dc0958020220ae Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Apr 2024 18:08:54 +1200 Subject: [PATCH 58/64] ldb-samba:ldif_handlers: dn_link_comparison: sort invalid DNs If both DNs are invalid, we can say they are equal. This means invalid or NULL DNs will sort to the end of the array, before deleted DNs: [ valid DNs, sorted | invalid/NULL DNs | deleted DNs, sorted ] BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 7280c8e53f463108fe3de443ce63572dde689a30) --- lib/ldb-samba/ldif_handlers.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/ldb-samba/ldif_handlers.c b/lib/ldb-samba/ldif_handlers.c index 90973cbf3c3..e339c1c8b10 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1179,12 +1179,18 @@ static int samba_ldb_dn_link_comparison(struct ldb_context *ldb, void *mem_ctx, } dn1 = ldb_dn_from_ldb_val(mem_ctx, ldb, v1); + dn2 = ldb_dn_from_ldb_val(mem_ctx, ldb, v2); + if ( ! ldb_dn_validate(dn1)) { TALLOC_FREE(dn1); + if ( ! ldb_dn_validate(dn2)) { + TALLOC_FREE(dn2); + return 0; + } + TALLOC_FREE(dn2); return 1; } - dn2 = ldb_dn_from_ldb_val(mem_ctx, ldb, v2); if ( ! ldb_dn_validate(dn2)) { TALLOC_FREE(dn1); TALLOC_FREE(dn2); -- 2.34.1 From a27c4bfada7ce415d15c4f28d9b40498dc2758f2 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 10 Apr 2024 10:54:41 +1200 Subject: [PATCH 59/64] 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 8688fe7a3475fe0908aaa2413c196b12b752c3ea Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 26 Apr 2024 15:24:47 +1200 Subject: [PATCH 60/64] 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 5bcc1c89068cc8ccbe16b43ec8979d7008618783 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 12 Apr 2024 18:11:47 +1200 Subject: [PATCH 61/64] s4:dsdb:mod: repl_md: make message_sort transitive Before we had (with a TODO of regret): if (!a1 || !a2) { return strcasecmp(e1->name, e2->name); } so, given {name:"A", id 2}, {name:"B", NO id}, {name:"C", id 1}, A < B by name B < C by name A > C by id Now the sort order is always A > C > B. This sort could have caused mysterious crashes in repl_meta_data if the schema is out of sync. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 5335f122fb551231a02a58f88f6a0aa23b5e02cb) --- source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index 1975c01b91d..762fe69f6c5 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -1063,12 +1063,20 @@ static int replmd_ldb_message_element_attid_sort(const struct ldb_message_elemen a2 = dsdb_attribute_by_lDAPDisplayName(schema, e2->name); /* - * TODO: remove this check, we should rely on e1 and e2 having valid attribute names - * in the schema + * If the elements do not have valid attribute names in the schema + * (which we would prefer to think can't happen), we need to sort them + * somehow. The current strategy is to put them at the end, sorted by + * attribute name. */ - if (!a1 || !a2) { + if (a1 == NULL && a2 == NULL) { return strcasecmp(e1->name, e2->name); } + if (a1 == NULL) { + return 1; + } + if (a2 == NULL) { + return -1; + } if (a1->attributeID_id == a2->attributeID_id) { return 0; } -- 2.34.1 From ddf3aac6b76ade2ab65ce3677df28b3b19e9f1d4 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 12 Apr 2024 20:28:04 +1200 Subject: [PATCH 62/64] s4:dsdb:mod: repl_md: message sort uses NUMERIC_CMP() No change at all in the result, just saving lines and branches. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 827b0c39ed0497407bfcfc5683735a165b1b0f0a) --- source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index 762fe69f6c5..4f2aefe7662 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -1077,10 +1077,7 @@ static int replmd_ldb_message_element_attid_sort(const struct ldb_message_elemen if (a2 == NULL) { return -1; } - if (a1->attributeID_id == a2->attributeID_id) { - return 0; - } - return a1->attributeID_id > a2->attributeID_id ? 1 : -1; + return NUMERIC_CMP(a1->attributeID_id, a2->attributeID_id); } static void replmd_ldb_message_sort(struct ldb_message *msg, -- 2.34.1 From f59afc9b069b991baa8ee7202b87d092c5ea9da9 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Apr 2024 13:21:25 +1200 Subject: [PATCH 63/64] 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 9d6b1872b4d28a9c921d6c8eb419231ec7fb4b11 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 26 Apr 2024 15:58:44 +1200 Subject: [PATCH 64/64] 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