This change seems to be addressing something different to the raised issue, and its not clear to me that this is correct. IIUC, it defers issuing the SO_PATH, LIST_ADD and LOAD commands to the dynamic engine until we read the next ctrl command from config...but what if there isn't a next ctrl command?

Would a more proper fix do the check in the dynamic_load() in eng_dyn.c?

The reason it was deferred is that in some cases the configuration defines the engine's "engine_id" only after it defines the "dynamic_path". That said, I understand your concern, and you are correct.

The problem was that we want the "engine_id" for our check, and if it is declared after "dynamic_path" we will already load an engine, and we won't be able to properly use the "ID" command to help the dynamic engine test if the engine exists before we load it.

If there is a way to tell the dynamic engine to attempt a load, while supplying it with the relevant "ID" information, the loading check could indeed be performed inside dynamic_load(). However, we still need to load the engine when the "engine_id" is known.

A possible fix could be to duplicate the code responsible for the dynamic load (lines 94-104), and placing it also after the for loop in case no load occurred. A sub-function could be used for this purpose to avoid code duplication.

Does this solution sound OK? If so, I'll upload a new version for the commit according to your suggestions.

I'd be fine with moving the check to dynamic_load().

I'll just ask to be sure I properly understood:

• Checking the engine_id could be done inside dynamic_load() and I'll move it there
• This check requires the dynamic_engine to use the "ID" control command
• The ID command should be used only when we have the "engine_id". In ibmca the config file looks like this:

dynamic_path = /usr/local/lib/ibmca.so
engine_id = ibmca

• This means that we can't load the engine the minute we see dynamic_path, and instead the engine should be loaded once we know both "dynamic_path" and "engine_id"
• For this purpose, I'm suggesting to start the dynamic load in the else case (as is in the commit today) and also at the end of the for loop (if the else case wasn't triggered).

Does it sound OK?

everything would be a lot easier if the engine_id and the dynamic_path statements were reversed,
like in test/recipes/90-test_gost_data/gost.conf:

[gost_section]
engine_id = gost
dynamic_path = $ENV::OPENSSL_GOST_ENGINE_SO
default_algorithms = ALL
CRYPT_PARAMS = id-Gost28147-89-CryptoPro-A-ParamSet

if that is written this way you would have the engine_id ready when dynamic_path is found.

I agree it will be easier, but it will break existing deployments, and this is why I tried to provide a solution that will work against existing configuration files.

in the case that the engine is not in the list, this does more than what your comment says:
this will try to load an engine with the given name in the engines directory.

Is there any way to query if a given engine (by engine_id) is already in the engine list? And if not, can I add one so it can be used for this purpose?

The problem is your patch creates a race condition between the query
for the duplicate name, and the ENGINE_ctrl_cmd_string(e, "LOAD", NULL, 0)
the engine with the same name may be loaded by a different thread, in between.

I understand your concern, and am working on a new revision for this commit in which the check is part of the "load" command in eng_dyn.c.

That said, the documentation for ENGINE_by_id() is: "Retrieve an engine from the list by its unique "id" value.", while in practice it also tries to load an engine with the given name from the engines directory. When checking for the existence of an engine, should I implement a new ENGINE_search() method?

As for the race condition, seeing that I will move the check to be inside the "load" cmd, it will resolve the issue of a 3rd party program loading the config after OpenSSL loaded it on its own, and it will drastically reduce the window for a race condition. The fix itself meant to be a robust workaround to the issue of crashing when loading the engines twice. That said, engines shouldn't get loaded twice in the first place, so if we do have a race condition that causes this scenario I think it is something we could live with.

yeah, something like an engine_search, with lowercase so it's internal.
one other concern that I have is that it may well be possible to use more than one dynamic_path
to load several engines, at once.

I didn't see any configuration file with such a setup, and anyway the configuration parser will only save the last line of a given key. I just tried to add multiple such lines to the file, and added a debug print and the for loop only showed the last "dynamic_path" line in the file. So we are covered.

I'll update my commit now, and upload it.

ah, good. thanks for double checking.

I've looked for where engine "LOAD" is used, and found another place:

Would it work to use the address of the bind function as an ID when the ENGINE_ctrl_cmd "ID" is not used ?