From 72045293bf0e8a9ce86a4a109cab65bc02a161eb 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 dabea68e15e7566ce5997d27f0eb2f3c3ef369c8 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 f0262e1f3f56a00f60ce801a209b57b9af0dad9e 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 d27feb365fafaf5c8ca63b2ebca83a2b8bf804bc 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 63d8aedd672..223686a3d83 100644 --- a/lib/ldb/include/ldb.h +++ b/lib/ldb/include/ldb.h @@ -2311,6 +2311,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 a8ee0f6b67e3c3ee1cffacb8d07dbeb24cd489a7 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 fb7a95bf04f..740b099e6be 100644 --- a/lib/ldb/common/ldb_dn.c +++ b/lib/ldb/common/ldb_dn.c @@ -1115,7 +1115,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 ba72188a72de8a238108559732093abd29738ed7 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 740b099e6be..c86a7f6692a 100644 --- a/lib/ldb/common/ldb_dn.c +++ b/lib/ldb/common/ldb_dn.c @@ -1194,7 +1194,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 94e56564e4a4c06dd25388afd42f5c9e1a65691f 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 783c4cd4b4964b06813678161e66b61892cd53d8 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 214079c0917..cd2e6fea9b9 100644 --- a/source4/dsdb/samdb/ldb_modules/operational.c +++ b/source4/dsdb/samdb/ldb_modules/operational.c @@ -1062,7 +1062,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 788d0145d2560da087f8e438004218b63122e3a1 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 625f454d1d0f9d28775b9665036b481e60bba435 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 fcd460dcd4c7957b1e43306149cc9a82b9069d5f 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. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (backported from commit ac0a8cd92ca4497bfcfad30e2b4d47547b582b92) [dbagnall@samba.org: master patch fixed 32 bit tests not present in 4.17] --- lib/util/charset/tests/charset.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 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; } -- 2.34.1 From 336f3ec57b6e7eeed859760c7a28385a51f78f80 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. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15625 Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (backported from commit dda0bb6fc71bae91f3158f69462cb79fdad210fb) [dbagnall@samba.org: master patch fixed 32 bit tests not present in 4.17] --- lib/util/charset/tests/charset.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 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; } -- 2.34.1 From c25d3c18d52224fb413266deda1c495d9c00be55 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 c211a2bb5704be2736bb9cf5426d662ac608d3b2 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 2a4ccd93673..ed0a7f3bee1 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 643abb6f3828603ec65d4493613782a8585c2c6c 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 a1826cd23b5c61ffdc3ee6578c1cf27872eedf1d 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 c35241e2983..faffd28cd25 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 97a87b19159870723cf3706b275a4f7abfb1a00d 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 faffd28cd25..b3abfa99938 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 b8079269a8a9451da2071441c2a55c61ade2148f 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 047fff22ad9..a6bd53bc8eb 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 41f831339a6ed3f0deb08b3be14e50885dea3830 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 a6bd53bc8eb..10acabad279 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 166ecccd7edf43ab2d3df1f3a158fcc3181a1c24 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 ea34c15e0e9..154b225db58 100644 --- a/lib/torture/torture.h +++ b/lib/torture/torture.h @@ -521,6 +521,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 af5c1d42d17b0fe5f4cc1f4f9f08ff6a9a0c2588 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 ed0a7f3bee1..163ba459010 100644 --- a/lib/util/charset/util_str.c +++ b/lib/util/charset/util_str.c @@ -156,14 +156,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 507d6cb654624f470465ca36ad89ad538628b1dc 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 febf2f414ca..4c60432b035 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 44574160ea5d93d02b5890e8ea6c1db3004b1316 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 4c60432b035..7e174366c53 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 8287baf235ecb94dded793cbc728923d35bfbcee 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 9c0900c10d534fe3502d6593d56f8f7d93396f52 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 2ea2cce2e83..f715b7cdcf3 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 c7b5a5ac736b48a10d77403b8f43a6ea215b6ebd 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 f715b7cdcf3..196a644f245 100644 --- a/lib/ldb/common/ldb_msg.c +++ b/lib/ldb/common/ldb_msg.c @@ -753,9 +753,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 cf5d73a3e8113dec47ed515d0b39967b8287a1a3 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 32bc3e187a7..b2ed1531df0 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 f6e5a8deeababa6b0fe964fc73a56edea18e2544 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 b2ed1531df0..711655ba326 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 46caf3893413fb9cf03740fd7d60e876534142b0 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 943847f04a3..c5ccf748ee4 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 72e4ad3f80b5e041afcb1622bcbe9a2981264ac2 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 b1342cbfe84..98cda8ad4b9 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 415856ee09852cbcecebe79417c28fc4e235cd83 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 3a41606439a8af50c01291bf40969fb75052993b 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 919fe74b35b9a7cf43b3d145e8c42e8e6a721517 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 46f92b34635ab9c9ddace654ef883af38ebc228a 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 c6cd7101c46023ad87555c32d7034d94009b4711 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 (backported from commit 5e99262aaf5fc6601f3859c8b060b680b11bf6ea) [dbagnall@samba.org: function has moved in master] --- source3/registry/reg_api_util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/registry/reg_api_util.c b/source3/registry/reg_api_util.c index eb83fa8f597..9424799a397 100644 --- a/source3/registry/reg_api_util.c +++ b/source3/registry/reg_api_util.c @@ -243,5 +243,5 @@ int registry_value_cmp(const struct registry_value* v1, const struct registry_va 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); } -- 2.34.1 From f9a0c9a67473ae79fc01d76ff69738ae5a0c1842 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 7ce7d2415dc..30b25d02d0b 100644 --- a/source3/utils/smbcacls.c +++ b/source3/utils/smbcacls.c @@ -553,22 +553,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 3a8f0c7eb05efb5cbd36efc9b07aee3675b1bd08 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 1c29f3864503fe9a988381a57b7d9ee03e0605a0 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 c8aa0743f54..d56279dbf73 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 1de0c4799c5f8400377baa9eba8afdd3fc2a5b09 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 8c76d3a7b218327f0e75aadfad5e36411ab16dde 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 80435057e321341c9bd3cecb8d19fa8f869f7d98 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 82f0f093ef355c0c58ed0b1744cc2d8fe8721e48 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 c86a7f6692a..2674094d7f6 100644 --- a/lib/ldb/common/ldb_dn.c +++ b/lib/ldb/common/ldb_dn.c @@ -1136,8 +1136,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 ad759ed21689172932557b1f6e4190a39acea491 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 68ade6b6d59..a91216f2e72 100644 --- a/source3/locking/brlock.c +++ b/source3/locking/brlock.c @@ -413,12 +413,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 5f954f9b79d23534d7784de498768079129494ac 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 74e25dc88c8c7d124fe6c49ce5d34064f5e2336c 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 ddd34ba812a..efd40849eb7 100644 --- a/source3/modules/vfs_vxfs.c +++ b/source3/modules/vfs_vxfs.c @@ -92,13 +92,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 9f48d0c261da247d4b051e642fdf72436bed6fab 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 03cf2405595..42a8657f1b7 100644 --- a/source4/dsdb/schema/schema_set.c +++ b/source4/dsdb/schema/schema_set.c @@ -436,19 +436,13 @@ static void dsdb_setup_attribute_shortcuts(struct ldb_context *ldb, struct dsdb_ } } -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) { @@ -465,11 +459,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) { @@ -477,7 +471,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); } /** -- 2.34.1 From 631618ab33c0a2a953837d85f021c5d8f3796676 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 88723a7786bfcac60338fd25a52b97decd16ea16 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 bd5b7259d3f..7c0d63564c2 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 cf392fa0b1dcc83cb65f82bad4f9cbf0f45406fb 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 3157e0cef12..b9c9f65a5ba 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 6f334246cbfc5ba60e2be65ebfd4bfde5a56ad50 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 607470f7e4e..4cfca7f369d 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 b1854f08bc3146887a820f962434d93909c828a4 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 1c2c3e38325ccdf8eae62f69b1282cd8710728c9 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 98cda8ad4b9..18c15ad2632 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 7146723d35d529a1ed5e23b96bcfa32d165f2759 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 78a433748bb..a3ea70c83b3 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 96fb0669f0a3fd96a9ca3134c3cd826c95c907f6 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 a3ea70c83b3..8c8370c0bac 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1152,13 +1152,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 3b47c1873f5158ed1a52c0071695096eb2ebb9e0 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 8c8370c0bac..78d712dc305 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1165,7 +1165,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 678ef78954bada3ffb1fc5c6da70ccd81201b0af 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 78d712dc305..a1256a9fce7 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1157,10 +1157,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 2af7e65b464555b2332863f91b0b974909b8a236 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 a1256a9fce7..8daf2e576f7 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1173,12 +1173,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 ee3ec919dead6f21b6ffced9ab051e98b8290dfa 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 8daf2e576f7..75dbf2170a0 100644 --- a/lib/ldb-samba/ldif_handlers.c +++ b/lib/ldb-samba/ldif_handlers.c @@ -1172,12 +1172,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 499af8300b79ab0ff494737fd481631923c07738 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 7e174366c53..2df2941c978 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 3b38bd6bd3b75c8ab0df034571d3529ac9c13dd5 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 2674094d7f6..00015d114e4 100644 --- a/lib/ldb/common/ldb_dn.c +++ b/lib/ldb/common/ldb_dn.c @@ -1145,13 +1145,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 762c6e9253f4bfa3d1af8c14f44db7649ae95211 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 175a02d3ba7..6892aa477b4 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -965,12 +965,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 aed7fc305b6a39ddf86358cdcc0e79983cb4bfa6 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 6892aa477b4..d574ba060a3 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -979,10 +979,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 728e0b9de102dac4966452a28eb50a92bb9e2afa 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 2df2941c978..86e16e4447f 100644 --- a/lib/ldb/common/attrib_handlers.c +++ b/lib/ldb/common/attrib_handlers.c @@ -420,7 +420,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 413ca0883bc28a7a436134eb8bcac1aa8d7003c6 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 (backported from commit e2051eebd492a419f840280336eb242d0b4a26ac) [dbagnall@samba.org: master has a different toupper() function] --- 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 86e16e4447f..04090a2b02f 100644 --- a/lib/ldb/common/attrib_handlers.c +++ b/lib/ldb/common/attrib_handlers.c @@ -388,17 +388,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)toupper(s1[n2]); + if (s1[n2] == '\0') { + return 0; + } + return 1; } else { - return -(int)toupper(s2[n1]); + if (s2[n1] == '\0') { + return 0; + } + return -1; } } return ret; -- 2.34.1