From: Bob Beck Date: Wed, 10 Sep 2025 17:43:01 +0000 (-0600) Subject: Ensure that empty or 1 element stacks are always sorted. X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=d88c43a64408616572941e5d0b127194d80f562f;p=thirdparty%2Fopenssl.git Ensure that empty or 1 element stacks are always sorted. Matt noticed "It's kind of weird that we are forced to call sort on a newly created and empty stack. It feels like an empty stack should have the "sorted" flag by default on creation" I am incluined to agree. This change ensures tht empty or 1 element stacks are marked as sorted, as per the existing comment in the file. Since this involved changing the various duplication routines to also ensure that sorted was preserved for such stacks, I also noticed the duplication code was largely duplicated. I took the opportunity to deduplicate the duplication code while making these changes. Reviewed-by: Matt Caswell Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/28509) --- diff --git a/crypto/stack/stack.c b/crypto/stack/stack.c index 543419ba06b..2efc0768e09 100644 --- a/crypto/stack/stack.c +++ b/crypto/stack/stack.c @@ -38,41 +38,54 @@ OPENSSL_sk_compfunc OPENSSL_sk_set_cmp_func(OPENSSL_STACK *sk, { OPENSSL_sk_compfunc old = sk->comp; - if (sk->comp != c) + if (sk->comp != c && sk->num > 1) sk->sorted = 0; sk->comp = c; return old; } -OPENSSL_STACK *OPENSSL_sk_dup(const OPENSSL_STACK *sk) +static OPENSSL_STACK *internal_copy(const OPENSSL_STACK *sk, + OPENSSL_sk_copyfunc copy_func, + OPENSSL_sk_freefunc free_func) { OPENSSL_STACK *ret; + int i; - if ((ret = OPENSSL_malloc(sizeof(*ret))) == NULL) + if ((ret = OPENSSL_sk_new_null()) == NULL) goto err; - if (sk == NULL) { - ret->num = 0; - ret->sorted = 0; - ret->comp = NULL; - } else { - /* direct structure assignment */ - *ret = *sk; - } + if (sk == NULL) + goto done; - if (sk == NULL || sk->num == 0) { - /* postpone |ret->data| allocation */ - ret->data = NULL; - ret->num_alloc = 0; - return ret; - } + /* direct structure assignment */ + *ret = *sk; + ret->data = NULL; + ret->num_alloc = 0; + + if (ret->num == 0) + goto done; /* nothing to copy */ - /* duplicate |sk->data| content */ - ret->data = OPENSSL_malloc_array(sk->num_alloc, sizeof(*ret->data)); + ret->num_alloc = ret->num > min_nodes ? ret->num : min_nodes; + ret->data = OPENSSL_calloc(ret->num_alloc, sizeof(*ret->data)); if (ret->data == NULL) goto err; - memcpy(ret->data, sk->data, sizeof(void *) * sk->num); + if (copy_func == NULL) { + memcpy(ret->data, sk->data, sizeof(*ret->data) * ret->num); + } else { + for (i = 0; i < ret->num; ++i) { + if (sk->data[i] == NULL) + continue; + if ((ret->data[i] = copy_func(sk->data[i])) == NULL) { + while (--i >= 0) + if (ret->data[i] != NULL) + free_func((void *)ret->data[i]); + goto err; + } + } + } + + done: return ret; err: @@ -84,48 +97,12 @@ OPENSSL_STACK *OPENSSL_sk_deep_copy(const OPENSSL_STACK *sk, OPENSSL_sk_copyfunc copy_func, OPENSSL_sk_freefunc free_func) { - OPENSSL_STACK *ret; - int i; - - if ((ret = OPENSSL_malloc(sizeof(*ret))) == NULL) - goto err; - - if (sk == NULL) { - ret->num = 0; - ret->sorted = 0; - ret->comp = NULL; - } else { - /* direct structure assignment */ - *ret = *sk; - } - - if (sk == NULL || sk->num == 0) { - /* postpone |ret| data allocation */ - ret->data = NULL; - ret->num_alloc = 0; - return ret; - } - - ret->num_alloc = sk->num > min_nodes ? sk->num : min_nodes; - ret->data = OPENSSL_calloc(ret->num_alloc, sizeof(*ret->data)); - if (ret->data == NULL) - goto err; - - for (i = 0; i < ret->num; ++i) { - if (sk->data[i] == NULL) - continue; - if ((ret->data[i] = copy_func(sk->data[i])) == NULL) { - while (--i >= 0) - if (ret->data[i] != NULL) - free_func((void *)ret->data[i]); - goto err; - } - } - return ret; + return internal_copy(sk, copy_func, free_func); +} - err: - OPENSSL_sk_free(ret); - return NULL; +OPENSSL_STACK *OPENSSL_sk_dup(const OPENSSL_STACK *sk) +{ + return internal_copy(sk, NULL, NULL); } OPENSSL_STACK *OPENSSL_sk_new_null(void) @@ -232,6 +209,7 @@ OPENSSL_STACK *OPENSSL_sk_new_reserve(OPENSSL_sk_compfunc c, int n) return NULL; st->comp = c; + st->sorted = 1; /* empty or single-element stack is considered sorted */ if (n <= 0) return st; @@ -286,7 +264,7 @@ int OPENSSL_sk_insert(OPENSSL_STACK *st, const void *data, int loc) st->data[loc] = data; } st->num++; - st->sorted = 0; + st->sorted = st->num <= 1; return st->num; } @@ -298,6 +276,7 @@ static ossl_inline void *internal_delete(OPENSSL_STACK *st, int loc) memmove(&st->data[loc], &st->data[loc + 1], sizeof(st->data[0]) * (st->num - loc - 1)); st->num--; + st->sorted = st->sorted || st->num <= 1; return (void *)ret; } @@ -487,7 +466,7 @@ void *OPENSSL_sk_set(OPENSSL_STACK *st, int i, const void *data) return NULL; } st->data[i] = data; - st->sorted = 0; + st->sorted = st->num <= 1; return (void *)st->data[i]; } diff --git a/test/stack_test.c b/test/stack_test.c index 74e1576c7c2..5a75d142be9 100644 --- a/test/stack_test.c +++ b/test/stack_test.c @@ -50,6 +50,15 @@ static int int_compare(const int *const *a, const int *const *b) return 0; } +static int int_compare_backward(const int *const *a, const int *const *b) +{ + if (**a > **b) + return -1; + if (**a < **b) + return 1; + return 0; +} + static int test_int_stack(int reserve) { static int v[] = { 1, 2, -4, 16, 999, 1, -173, 1, 9 }; @@ -90,6 +99,9 @@ static int test_int_stack(int reserve) /* Check push and num */ for (i = 0; i < n; i++) { + /* Empty or single element stack should be sorted */ + if ((i == 0 || i == 1) && !TEST_true(sk_sint_is_sorted(s))) + goto end; if (!TEST_int_eq(sk_sint_num(s), i)) { TEST_info("int stack size %d", i); goto end; @@ -134,9 +146,18 @@ static int test_int_stack(int reserve) /* sorting */ if (!TEST_false(sk_sint_is_sorted(s))) goto end; + (void)sk_sint_set_cmp_func(s, &int_compare_backward); + sk_sint_sort(s); + if (!TEST_true(sk_sint_is_sorted(s))) /* should be sorted */ + goto end; + (void)sk_sint_set_cmp_func(s, &int_compare_backward); + if (!TEST_true(sk_sint_is_sorted(s))) /* should still be sorted */ + goto end; (void)sk_sint_set_cmp_func(s, &int_compare); + if (!TEST_false(sk_sint_is_sorted(s))) /* should no longer be sorted */ + goto end; sk_sint_sort(s); - if (!TEST_true(sk_sint_is_sorted(s))) + if (!TEST_true(sk_sint_is_sorted(s))) /* now should be sorted again */ goto end; /* find sorted -- the value is matched so we don't need to locate it */ @@ -178,7 +199,7 @@ static int test_uchar_stack(int reserve) { static const unsigned char v[] = { 1, 3, 7, 5, 255, 0 }; const int n = OSSL_NELEM(v); - STACK_OF(uchar) *s = sk_uchar_new(&uchar_compare), *r = NULL; + STACK_OF(uchar) *s = sk_uchar_new(&uchar_compare), *r = NULL, *q = NULL; int i; int testresult = 0; @@ -205,19 +226,40 @@ static int test_uchar_stack(int reserve) r = sk_uchar_dup(s); if (!TEST_int_eq(sk_uchar_num(r), n)) goto end; + q = sk_uchar_dup(s); + if (!TEST_int_eq(sk_uchar_num(q), n)) + goto end; sk_uchar_sort(r); + sk_uchar_sort(q); /* pop */ - for (i = 0; i < n; i++) + for (i = 0; i < n; i++) { if (!TEST_ptr_eq(sk_uchar_pop(s), v + i)) { TEST_info("uchar pop %d", i); goto end; } + /* Previously unsorted stack of more than 1 element remains unsorted */ + if (i < n - 2 && !TEST_false(sk_uchar_is_sorted(s))) + goto end; + /* A single or zero element stack should be sorted */ + if (i > n - 2 && !TEST_true(sk_uchar_is_sorted(s))) + goto end; + } /* free -- we rely on the debug malloc to detect leakage here */ sk_uchar_free(s); s = NULL; + /* pop */ + for (i = 0; i < n; i++) { + /* A sorted stack should remain sorted */ + if (!TEST_true(sk_uchar_is_sorted(q))) + goto end; + } + /* free -- we rely on the debug malloc to detect leakage here */ + sk_uchar_free(q); + q = NULL; + /* dup again */ if (!TEST_int_eq(sk_uchar_num(r), n)) goto end;