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 <matt@openssl.org>
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
(Merged from https://github.com/openssl/openssl/pull/28509)

diff --git a/crypto/stack/stack.c b/crypto/stack/stack.c
index 543419ba06bc537ccee49defe150afba5f67e089..2efc0768e0964228d3d0d20eab6fdcea3dc2edfd 100644 (file)
--- 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 74e1576c7c2405b3be6f2220f70f524e5958d080..5a75d142be963ced8c9e619e744d221a668951ff 100644 (file)
--- 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;