Fix engine cleanup error handling

Error handling in engine_cleanup_add_first/last was
broken and caused memory leaks.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/21971)

diff --git a/crypto/engine/eng_lib.c b/crypto/engine/eng_lib.c
index 8345f684c813f5c72dad27e1cd080ff61579c1b6..412363fa371e3f9da2efdf96bac6bee29ac7cd66 100644 (file)
--- a/crypto/engine/eng_lib.c
+++ b/crypto/engine/eng_lib.c
@@ -135,28 +135,34 @@ static ENGINE_CLEANUP_ITEM *int_cleanup_item(ENGINE_CLEANUP_CB *cb)
     return item;
 }
 
-void engine_cleanup_add_first(ENGINE_CLEANUP_CB *cb)
+int engine_cleanup_add_first(ENGINE_CLEANUP_CB *cb)
 {
     ENGINE_CLEANUP_ITEM *item;
 
     if (!int_cleanup_check(1))
-        return;
+        return 0;
     item = int_cleanup_item(cb);
-    if (item != NULL)
-        if (sk_ENGINE_CLEANUP_ITEM_insert(cleanup_stack, item, 0) <= 0)
-            OPENSSL_free(item);
+    if (item != NULL) {
+        if (sk_ENGINE_CLEANUP_ITEM_insert(cleanup_stack, item, 0))
+            return 1;
+        OPENSSL_free(item);
+    }
+    return 0;
 }
 
-void engine_cleanup_add_last(ENGINE_CLEANUP_CB *cb)
+int engine_cleanup_add_last(ENGINE_CLEANUP_CB *cb)
 {
     ENGINE_CLEANUP_ITEM *item;
+
     if (!int_cleanup_check(1))
-        return;
+        return 0;
     item = int_cleanup_item(cb);
     if (item != NULL) {
-        if (sk_ENGINE_CLEANUP_ITEM_push(cleanup_stack, item) <= 0)
-            OPENSSL_free(item);
+        if (sk_ENGINE_CLEANUP_ITEM_push(cleanup_stack, item) > 0)
+            return 1;
+        OPENSSL_free(item);
     }
+    return 0;
 }
 
 /* The API function that performs all cleanup */

diff --git a/crypto/engine/eng_list.c b/crypto/engine/eng_list.c
index 119e1c60459a8d39eed6b54ad3b554130368832f..a2c151d64a04ae13ee492fc32c429b4b5ae83bda 100644 (file)
--- a/crypto/engine/eng_list.c
+++ b/crypto/engine/eng_list.c
@@ -89,12 +89,16 @@ static int engine_list_add(ENGINE *e)
             ERR_raise(ERR_LIB_ENGINE, ENGINE_R_INTERNAL_LIST_ERROR);
             return 0;
         }
-        engine_list_head = e;
-        e->prev = NULL;
         /*
          * The first time the list allocates, we should register the cleanup.
          */
-        engine_cleanup_add_last(engine_list_cleanup);
+        if (!engine_cleanup_add_last(engine_list_cleanup)) {
+            CRYPTO_DOWN_REF(&e->struct_ref, &ref);
+            ERR_raise(ERR_LIB_ENGINE, ENGINE_R_INTERNAL_LIST_ERROR);
+            return 0;
+        }
+        engine_list_head = e;
+        e->prev = NULL;
     } else {
         /* We are adding to the tail of an existing list. */
         if ((engine_list_tail == NULL) || (engine_list_tail->next != NULL)) {

diff --git a/crypto/engine/eng_local.h b/crypto/engine/eng_local.h
index 6f5d380d02a43e647a48fdc6f2c48214daf19162..24920973e7b53e60b0171aa8b11984acdad8f12d 100644 (file)
--- a/crypto/engine/eng_local.h
+++ b/crypto/engine/eng_local.h
@@ -46,8 +46,8 @@ typedef struct st_engine_cleanup_item {
     ENGINE_CLEANUP_CB *cb;
 } ENGINE_CLEANUP_ITEM;
 DEFINE_STACK_OF(ENGINE_CLEANUP_ITEM)
-void engine_cleanup_add_first(ENGINE_CLEANUP_CB *cb);
-void engine_cleanup_add_last(ENGINE_CLEANUP_CB *cb);
+int engine_cleanup_add_first(ENGINE_CLEANUP_CB *cb);
+int engine_cleanup_add_last(ENGINE_CLEANUP_CB *cb);
 
 /* We need stacks of ENGINEs for use in eng_table.c */
 DEFINE_STACK_OF(ENGINE)

diff --git a/crypto/engine/eng_table.c b/crypto/engine/eng_table.c
index 17225d0ad44cb6261f8c8e9c2ad960fe4cf4d5b7..3138a1526002a70af59791d082e2746059440005 100644 (file)
--- a/crypto/engine/eng_table.c
+++ b/crypto/engine/eng_table.c
@@ -93,9 +93,11 @@ int engine_table_register(ENGINE_TABLE **table, ENGINE_CLEANUP_CB *cleanup,
         added = 1;
     if (!int_table_check(table, 1))
         goto end;
-    if (added)
-        /* The cleanup callback needs to be added */
-        engine_cleanup_add_first(cleanup);
+    /* The cleanup callback needs to be added */
+    if (added && !engine_cleanup_add_first(cleanup)) {
+        lh_ENGINE_PILE_free(&(*table)->piles);
+        *table = NULL;
+    }
     while (num_nids--) {
         tmplate.nid = *nids;
         fnd = lh_ENGINE_PILE_retrieve(&(*table)->piles, &tmplate);