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 openssl#28509)

Author: bob-beck

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];
 }
 

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;