stack: Do not add error if pop/shift/value accesses outside of the stack

This partially reverts commit 30eba7f35983a917f1007bce45040c0af3442e42.
This is legitimate use of the stack functions and no error
should be reported apart from the NULL return value.

Fixes #19389

Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/19400)

diff --git a/crypto/conf/conf_def.c b/crypto/conf/conf_def.c
index b6b56cd9679ff9d089b05e776f676a1f5aa931eb..5e81d9e941ae7dcb9e9ba8c3d97175be77d47970 100644 (file)
--- a/crypto/conf/conf_def.c
+++ b/crypto/conf/conf_def.c
@@ -294,7 +294,7 @@ static int def_load_bio(CONF *conf, BIO *in, long *line)
             }
 #endif
             /* no more files in directory, continue with processing parent */
-            if (sk_BIO_num(biosk) < 1 || (parent = sk_BIO_pop(biosk)) == NULL) {
+            if ((parent = sk_BIO_pop(biosk)) == NULL) {
                 /* everything processed get out of the loop */
                 break;
             } else {

diff --git a/crypto/stack/stack.c b/crypto/stack/stack.c
index 7e1c24515cbf65dea70e950f366069d1ee5a773f..dc1c7d36d07827427ff2109c60f715c48325f479 100644 (file)
--- a/crypto/stack/stack.c
+++ b/crypto/stack/stack.c
@@ -297,6 +297,9 @@ void *OPENSSL_sk_delete_ptr(OPENSSL_STACK *st, const void *p)
 {
     int i;
 
+    if (st == NULL)
+        return NULL;
+
     for (i = 0; i < st->num; i++)
         if (st->data[i] == p)
             return internal_delete(st, i);
@@ -305,15 +308,8 @@ void *OPENSSL_sk_delete_ptr(OPENSSL_STACK *st, const void *p)
 
 void *OPENSSL_sk_delete(OPENSSL_STACK *st, int loc)
 {
-    if (st == NULL) {
-        ERR_raise(ERR_LIB_X509, ERR_R_PASSED_NULL_PARAMETER);
+    if (st == NULL || loc < 0 || loc >= st->num)
         return NULL;
-    }
-    if (loc < 0 || loc >= st->num) {
-        ERR_raise_data(ERR_LIB_X509, ERR_R_PASSED_INVALID_ARGUMENT,
-                       "loc=%d", loc);
-        return NULL;
-    }
 
     return internal_delete(st, loc);
 }
@@ -397,37 +393,21 @@ int OPENSSL_sk_unshift(OPENSSL_STACK *st, const void *data)
 
 void *OPENSSL_sk_shift(OPENSSL_STACK *st)
 {
-    if (st == NULL) {
-        ERR_raise(ERR_LIB_X509, ERR_R_PASSED_NULL_PARAMETER);
-        return NULL;
-    }
-    if (st->num == 0) {
-        ERR_raise(ERR_LIB_X509, ERR_R_PASSED_INVALID_ARGUMENT);
+    if (st == NULL || st->num == 0)
         return NULL;
-    }
     return internal_delete(st, 0);
 }
 
 void *OPENSSL_sk_pop(OPENSSL_STACK *st)
 {
-    if (st == NULL) {
-        ERR_raise(ERR_LIB_X509, ERR_R_PASSED_NULL_PARAMETER);
-        return NULL;
-    }
-    if (st->num == 0) {
-        ERR_raise(ERR_LIB_X509, ERR_R_PASSED_INVALID_ARGUMENT);
+    if (st == NULL || st->num == 0)
         return NULL;
-    }
     return internal_delete(st, st->num - 1);
 }
 
 void OPENSSL_sk_zero(OPENSSL_STACK *st)
 {
-    if (st == NULL) {
-        ERR_raise(ERR_LIB_X509, ERR_R_PASSED_NULL_PARAMETER);
-        return;
-    }
-    if (st->num == 0)
+    if (st == NULL || st->num == 0)
         return;
     memset(st->data, 0, sizeof(*st->data) * st->num);
     st->num = 0;
@@ -460,15 +440,8 @@ int OPENSSL_sk_num(const OPENSSL_STACK *st)
 
 void *OPENSSL_sk_value(const OPENSSL_STACK *st, int i)
 {
-    if (st == NULL) {
-        ERR_raise(ERR_LIB_X509, ERR_R_PASSED_NULL_PARAMETER);
-        return NULL;
-    }
-    if (i < 0 || i >= st->num) {
-        ERR_raise_data(ERR_LIB_X509, ERR_R_PASSED_INVALID_ARGUMENT,
-                       "i=%d", i);
+    if (st == NULL || i < 0 || i >= st->num)
         return NULL;
-    }
     return (void *)st->data[i];
 }
 

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index b2a68c9f348dcb8dd022837b4c6191ca39886c22..3b3eda4001d842cb86522c7fdf1dfb1bee6073e2 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -5787,8 +5787,7 @@ static int ct_move_scts(STACK_OF(SCT) **dst, STACK_OF(SCT) *src,
         }
     }
 
-    while (sk_SCT_num(src) > 0) {
-        sct = sk_SCT_pop(src);
+    while ((sct = sk_SCT_pop(src)) != NULL) {
         if (SCT_set_source(sct, origin) != 1)
             goto err;
 

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 75f71fbd81ed70e44dba0c4d27de3cac6f7cf596..6d4be61118bf0ba378fd71a15aa757adb76c70f6 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -3659,7 +3659,7 @@ MSG_PROCESS_RETURN tls_process_client_certificate(SSL_CONNECTION *s,
     }
 
     X509_free(s->session->peer);
-    s->session->peer = sk_X509_num(sk) == 0 ? NULL: sk_X509_shift(sk);
+    s->session->peer = sk_X509_shift(sk);
     s->session->verify_result = s->verify_result;
 
     OSSL_STACK_OF_X509_free(s->session->peer_chain);

diff --git a/test/helpers/ssltestlib.c b/test/helpers/ssltestlib.c
index 13564a08a71364f23fda5f7029b26d7776aa2d84..727ba7ff6ae279349df7e95b7ccd6044f01d2702 100644 (file)
--- a/test/helpers/ssltestlib.c
+++ b/test/helpers/ssltestlib.c
@@ -349,8 +349,7 @@ static int mempacket_test_read(BIO *bio, char *out, int outl)
     unsigned int seq, offset, len, epoch;
 
     BIO_clear_retry_flags(bio);
-    if (sk_MEMPACKET_num(ctx->pkts) <= 0
-        || (thispkt = sk_MEMPACKET_value(ctx->pkts, 0)) == NULL
+    if ((thispkt = sk_MEMPACKET_value(ctx->pkts, 0)) == NULL
         || thispkt->num != ctx->currpkt) {
         /* Probably run out of data */
         BIO_set_retry_read(bio);
@@ -603,9 +602,8 @@ int mempacket_test_inject(BIO *bio, const char *in, int inl, int pktnum,
             ctx->lastpkt++;
             do {
                 i++;
-                if (i < sk_MEMPACKET_num(ctx->pkts)
-                        && (nextpkt = sk_MEMPACKET_value(ctx->pkts, i)) != NULL
-                        && nextpkt->num == ctx->lastpkt)
+                nextpkt = sk_MEMPACKET_value(ctx->pkts, i);
+                if (nextpkt != NULL && nextpkt->num == ctx->lastpkt)
                     ctx->lastpkt++;
                 else
                     return inl;