Fix engine cleanup error handling

[bernd-edlinger]

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

[t8m]

Would it be possible to make test for this?

[bernd-edlinger]

Would it be possible to make test for this?

The test that found it, is already there.

I did it using ./config enable-crypto-mdebug enable-crypto-mdebug-backtrace
together with OPENSSL_MALLOC_FAILURES=1@1 make test V=1
and spotted it, by looking at the keyword "bytes leaked",
ignoring lots of irrelevant findings outside the crypto tree.

Unfortunately the feature crypto/mem_dbg.c was removed in 3.0,
but address sanitizer cannot replace that functionality completely,
so enable-crypto-mdebug is crippled and enable-crypto-mdebug-backtrace
is currently useless.
Therefore my testing had to be done against the 1.1.1 branch.

[bernd-edlinger]

@openssl/committers ping...

[openssl-machine]

This pull request is ready to merge

[bernd-edlinger]

Merged to all branches. Thanks!

[tom-cosgrove-arm]

The merge to 3.0 has broken the 3.0 branch

crypto/engine/eng_list.c:85:49: error: too few arguments provided to function-like macro invocation
            CRYPTO_DOWN_REF(&e->struct_ref, &ref);
                                                ^
include/internal/refcount.h:162:10: note: macro 'CRYPTO_DOWN_REF' defined here
# define CRYPTO_DOWN_REF(val, ret, lock) CRYPTO_atomic_add(val, -1, ret, lock)
         ^

as the declaration of ref was not brought in

diff --git a/crypto/engine/eng_list.c b/crypto/engine/eng_list.c
index 04c73c7628..d3bd0c0dc2 100644
--- a/crypto/engine/eng_list.c
+++ b/crypto/engine/eng_list.c
@@ -78,12 +78,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)) {

[bernd-edlinger]

ah sorry, now I see 3.1 is actually also broken, since the struct ref is incremented below, while master incremended it before.

[tom-cosgrove-arm]

(I didn't check 3.1; I saw the 3.0 failure on the fuzz results on another PR)

[bernd-edlinger]

quite possible that 3.1 will compile, but the ref-counting is weird on master, that confused me a bit...