Merge 'sverker/crypto/ensure-engine-unloaded-fix/OTP-17593' into maint

4 files changed, 64 insertions, 46 deletions

diff --git a/lib/crypto/c_src/crypto.c b/lib/crypto/c_src/crypto.c
index 5c0a1ccf6a..16d1e3a020 100644
--- a/lib/crypto/c_src/crypto.c
+++ b/lib/crypto/c_src/crypto.c
@@ -111,7 +111,6 @@ static ErlNifFunc nif_funcs[] = {
 
     {"engine_by_id_nif", 1, engine_by_id_nif, 0},
     {"engine_init_nif", 1, engine_init_nif, 0},
-    {"engine_finish_nif", 1, engine_finish_nif, 0},
     {"engine_free_nif", 1, engine_free_nif, 0},
     {"engine_load_dynamic_nif", 0, engine_load_dynamic_nif, 0},
     {"engine_ctrl_cmd_strings_nif", 3, engine_ctrl_cmd_strings_nif, 0},
diff --git a/lib/crypto/c_src/engine.c b/lib/crypto/c_src/engine.c
index 82ba50e4bd..263acd483d 100644
--- a/lib/crypto/c_src/engine.c
+++ b/lib/crypto/c_src/engine.c
@@ -208,44 +208,27 @@ ERL_NIF_TERM engine_free_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]
     // Get Engine
     ASSERT(argc == 1);
 
-    if (!enif_get_resource(env, argv[0], engine_ctx_rtype, (void**)&ctx)
-        || ctx->is_functional)
-        goto bad_arg;
-
-    if (!ENGINE_free(ctx->engine))
-        goto err;
-    ctx->engine = NULL;
-    return atom_ok;
-
- bad_arg:
- err:
-    return enif_make_badarg(env);
-#else
-    return atom_notsup;
-#endif
-}
-
-ERL_NIF_TERM engine_finish_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
-{/* (Engine) */
-#ifdef HAS_ENGINE_SUPPORT
-    struct engine_ctx *ctx;
-
-    // Get Engine
-    ASSERT(argc == 1);
-
-    if (!enif_get_resource(env, argv[0], engine_ctx_rtype, (void**)&ctx)
-        || !ctx->is_functional)
+    if (!enif_get_resource(env, argv[0], engine_ctx_rtype, (void**)&ctx))
         goto bad_arg;
 
-    if (!ENGINE_finish(ctx->engine))
-        goto err;
-    ctx->is_functional = 0;
+    if (ctx->engine) {
+        if (ctx->is_functional) {
+            if (!ENGINE_finish(ctx->engine))
+                goto err;
+            ctx->is_functional = 0;
+        }
+        if (!ENGINE_free(ctx->engine))
+            goto err;
+        ctx->engine = NULL;
+    }
+    else {
+        ASSERT(!ctx->is_functional);
+    }
     return atom_ok;
 
  bad_arg:
  err:
     return enif_make_badarg(env);
-
 #else
     return atom_notsup;
 #endif
@@ -645,6 +628,10 @@ ERL_NIF_TERM engine_get_next_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM ar
         || !ctx->engine)
         goto bad_arg;
 
+    if (ctx->is_functional) {
+        ENGINE_finish(ctx->engine);
+        ctx->is_functional = 0;
+    }
     engine = ENGINE_get_next(ctx->engine);
     ctx->engine = NULL;
 
diff --git a/lib/crypto/src/crypto.erl b/lib/crypto/src/crypto.erl
index d917caeae9..6a64774f71 100644
--- a/lib/crypto/src/crypto.erl
+++ b/lib/crypto/src/crypto.erl
@@ -1781,7 +1781,7 @@ engine_load_2(Engine, PostCmds, EngineMethods) ->
     catch
        throw:Error ->
           %% The engine registration failed, release the functional reference
-          ok = engine_finish_nif(Engine),
+          ok = engine_free_nif(Engine),
           throw(Error)
     end.
 
@@ -1800,9 +1800,7 @@ engine_unload(Engine, EngineMethods) ->
     try
         [ok = engine_nif_wrapper(engine_unregister_nif(Engine, engine_method_atom_to_int(Method))) ||
             Method <- EngineMethods],
-        %% Release the functional reference from engine_init_nif
-        ok = engine_nif_wrapper(engine_finish_nif(Engine)),
-        %% Release the structural reference from engine_by_id_nif
+        %% Release the reference from engine_by_id_nif
         ok = engine_nif_wrapper(engine_free_nif(Engine))
     catch
        throw:Error ->
@@ -1981,7 +1979,7 @@ ensure_engine_loaded_2(Engine, Methods) ->
     catch
        throw:Error ->
           %% The engine registration failed, release the functional reference
-          ok = engine_finish_nif(Engine),
+          ok = engine_free_nif(Engine),
           throw(Error)
     end.
 %%----------------------------------------------------------------------
@@ -2000,13 +1998,21 @@ ensure_engine_unloaded(Engine) ->
                                                 EngineMethods :: [engine_method_type()],
                                                 Result :: ok | {error, Reason::term()}.
 ensure_engine_unloaded(Engine, EngineMethods) ->
-    case engine_remove(Engine) of
-        ok ->
-            engine_unload(Engine, EngineMethods);
-        {error, E} ->
-            {error, E}
+    List = crypto:engine_list(),
+    EngineId = crypto:engine_get_id(Engine),
+    case lists:member(EngineId, List) of
+        true ->
+            case engine_remove(Engine) of
+                ok ->
+                    engine_unload(Engine, EngineMethods);
+                {error, Error} ->
+                    {error, Error}
+            end;
+        false ->
+            engine_unload(Engine, EngineMethods)
     end.
 
+
 %%--------------------------------------------------------------------
 %%% On load
 %%--------------------------------------------------------------------
@@ -2395,7 +2401,6 @@ packed_openssl_version(MAJ, MIN, FIX, P0) ->
 %% Engine nifs
 engine_by_id_nif(_EngineId) -> ?nif_stub.
 engine_init_nif(_Engine) -> ?nif_stub.
-engine_finish_nif(_Engine) -> ?nif_stub.
 engine_free_nif(_Engine) -> ?nif_stub.
 engine_load_dynamic_nif() -> ?nif_stub.
 engine_ctrl_cmd_strings_nif(_Engine, _Cmds, _Optional) -> ?nif_stub.
diff --git a/lib/crypto/test/engine_SUITE.erl b/lib/crypto/test/engine_SUITE.erl
index de902ca3f0..e4bba9c3a8 100644
--- a/lib/crypto/test/engine_SUITE.erl
+++ b/lib/crypto/test/engine_SUITE.erl
@@ -715,8 +715,8 @@ ensure_load(Config) when is_list(Config) ->
                 Md5Hash1 = crypto:hash(md5, "Don't panic"),
                 Md5Hash2 =  <<0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15>>,
                 case crypto:ensure_engine_loaded(<<"MD5">>, Engine) of
-                    {ok, E} ->
-                        {ok, _E1} = crypto:ensure_engine_loaded(<<"MD5">>, Engine),
+                    {ok, E1} ->
+                        {ok, E2} = crypto:ensure_engine_loaded(<<"MD5">>, Engine),
                         case crypto:hash(md5, "Don't panic") of
                             Md5Hash1 ->
                                 ct:fail(fail_to_load_still_original_engine);
@@ -725,11 +725,37 @@ ensure_load(Config) when is_list(Config) ->
                             _ ->
                                 ct:fail(fail_to_load_engine)
                         end,
-                        ok = crypto:ensure_engine_unloaded(E),
+
+                        {ok, E3} = crypto:engine_by_id(<<"MD5">>),
+
+                        ok = crypto:ensure_engine_unloaded(E3),
+                        case crypto:hash(md5, "Don't panic") of
+                            Md5Hash1 ->
+                                ok;
+                            Md5Hash2 ->
+                                ct:fail(fail_to_unload_still_test_engine);
+                            _ ->
+                                ct:fail(load_engine)
+                        end,
+
+                        %% ToDo: Why doesn't this work?
+                        %% {ok, E4} = crypto:ensure_engine_loaded(<<"MD5">>, Engine),
+                        %% case crypto:hash(md5, "Don't panic") of
+                        %%     Md5Hash1 ->
+                        %%         ct:fail(fail_to_load_still_original_engine);
+                        %%     Md5Hash2 ->
+                        %%         ok;
+                        %%     _ ->
+                        %%         ct:fail(fail_to_load_engine)
+                        %% end,
+
+                        ok = crypto:ensure_engine_unloaded(E1),
                         case crypto:hash(md5, "Don't panic") of
                             Md5Hash2 ->
                                 ct:fail(fail_to_unload_still_test_engine);
                             Md5Hash1 ->
+                                ok = crypto:ensure_engine_unloaded(E2),
+                                %% ok = crypto:ensure_engine_unloaded(E4);
                                 ok;
                             _ ->
                                 ct:fail(fail_to_unload_engine)
@@ -743,6 +769,7 @@ ensure_load(Config) when is_list(Config) ->
            end
     end.
 
+
 %%%----------------------------------------------------------------
 %%% Pub/priv key storage tests.  Those are for testing the crypto.erl
 %%% support for using priv/pub keys stored in an engine.