Refactor the ex_data initialization to allow multiple inits#8828
Refactor the ex_data initialization to allow multiple inits#8828levitte wants to merge 1 commit intoopenssl:masterfrom
Conversation
With multiple library contexts, it's possible that the same module runs its init and teardown functions multiple times during its lifetime. This would normally not cause much trouble, except that our FIPS module is a bit special and will include code that handles a library context internally, just to be able to make internal EVP calls. This means that the provider init function will potentially be called multiple times, and the teardown as well, and those may do initializations and deinitializations, including the ex_data cleanup. This may mean that the ex_data lock gets freed too early (on first cleanup instead of last). To ensure that the ex_data lock isn't freed before its last use, we associate a reference counter with it, which gets incremented for everytime there's an attempt to create that lock, and gets decremented when there's an attempt to clean it up, thereby ensuring that construction will only happen on first init and destruction on last deinit. Related to openssl#8728
paulidale
added
the
approval: done
|
Travis failures appear relevant. |
| int i; | ||
|
|
||
| return RUN_ONCE(&ex_data_init_flag, do_ex_data_init) | ||
| && CRYPTO_UP_REF(&ex_data_lock_count, &i, ex_data_lock_count_lock); |
There was a problem hiding this comment.
This doesn't look right. We'll end up incrementing the ref count every time someone calls get_and_lock.
There was a problem hiding this comment.
Hmmm, you're right. Damnit!
This is quite infuriating, it seems that we're fighting a losing battle to avoid having to deal with shared object init/fini, DllMain or whatever different platforms use, while trying to perform exactly what they are designed for.
There was a problem hiding this comment.
Hey....did I ever mention openssl_ctx_runone()... ;-)
|
Aren't you going to have to do this for every module that has an init? |
Not really. This FIPS module is a bit of a special case. Most modules have no business having their own copy of the ex_data code. Either they won't use it at all, or they will link with libcrypto and use the libcrypto code for it. This is only necessary because we're bringing this stuff inside the module boundary. |
|
Or you expose a few callbacks for get-index/fetch/store and let the module call them into the core? |
|
I am concerned how two copies of the exdata stuff will work on platforms where DLL's are in a separate memory address space. For example, Windows. |
libcrypto will have its own ex_data and the FIPS module will have its ex_data. That's what we want, isn't it? |
|
who allocates the exdata array in an object -- which malloc, and how do you know to free it? we want modules to have their own INSTANCES of ex_data, but not the infrastructure; that should (must) be shared. |
Wellllll, depends. We're dealing with a special case, the FIPS module, which has its own copy of everything it needs, and that turned out to include the ex_data stuff (which it would have either way, sooner or later). The FIPS module will not share anything with libcrypto, i.e. the application. |
No - that absolutely must not be shared in the FIPS module. Or rather, the source can be shared, but libcrypto and the FIPS module have to have their own independent copies. You cannot leave the FIPS module boundary to do crypto work. |
|
You can leave the FIPS module boundary to do things like allocate memory and assign pointers; malloc, etc. The crypto code must be within the module. I suggest you ask the lab to confirm your conclusion, and to understand why error-stack callbacks are okay, but not pointer-allocation. |
|
What will happen on windows if I create a FIPS-managed object and my application adds some exdata, and then I free the FIPS object. Which part, core or module, free's which resources? |
|
How will you application add exdata to the provider side object? As far as I know, there's no interface to do so, so I wonder how you will accomplish that. |
|
I've been giving this PR quite a bit of thought, and found that this isn't the right spot for this approach. Closing it without merge. |
|
To answer your question: the application will still have an RSA object, or an X509 object, or an SSL object, and it can make ex_data calls there, no? |
|
Yes. But that's not provider side data, at least so far. |
|
The list of pointers in the ex_data is currently planned to be in the provider and the unique ex_data numeric index could now be not unique (since it exists in the provider and the core). I think both of those things are a mistake, and I have tried to point it out before. |
|
You're mixing things up. OPENSSL_CTX is implemented as an ex_data, but it's not tied to any other object (so definitely not part of a RSA object, X509 object, ...) |
|
No, I am not mixing things up. I am talking only about ex_data and unique indices. I am sorry if I am not making myself clear. |
|
Well... Regarding ex_data attached to an RSA structure (or even an EVP_PKEY, if there is an ex_data in that structure), there's no reason whatsoever why it should be passed to the provider. Regarding indices, regardless of where they are created, they will be created in the entity (shared library, module) that has the functionality. If that functionality is in a module (such as the FIPS module), then the identities will be module local. |
|
Only a module can add exdata? Can an application add exdata? How do you keep those from getting entangled, such as by re-using an index (one created in-module one created in the core)? |
Objects do not cross the libcrypto<->provider boundary. You cannot (for example) create an RSA object in libcrypto and pass it to the provider. Similarly objects created in the provider don't get passed back to libcrypto. All we do is pass "void *" pointers around. If we need to pass an RSA object it would get converted into an OSSL_PARAM and that would get passed. There is no possibility of ex_data on the provider side getting confused with libcrypto side ex_data. They are entirely separate. |
This confuses me. Please show sample code for how an application creates a FIPS-module RSA keypair. |
Roughly speaking, the application would call In other words, in a provider-powered OpenSSL, an |
|
But |
Huh? Here's what I wrote:
|
|
As far as the application is concerned it would have an EVP_PKEY and can do everything with an EVP_PKEY that it normally would. If you look inside that EVP_PKEY though you won't find an RSA object anyway. All you will find is a void * pointer. Any function calls that need to do RSA type things get delegated down to the provider and the void * pointer is passed. On the provider side that void * gets cast into a provider side RSA object and it services the requests as required. The provider side RSA object never leaves the provider except as a void * pointer. |
Actually its probably more accurate to say that it gets cast into some provider side equivalent of the EVP_PKEY object that contains the RSA object. |
Uhmmm, I think this is saying too much, conceptually speaking. You have no idea how the provider module for BLARGH will look, all you know is that this |
|
BTW, I've reasons to believe this should be re-opened, and then reworked slightly. |
levitte
dismissed
paulidale’s stale review
Dismiss approval because rework is needed
|
No. I'm deciding finally that no. There's a fundamental init problem that this can't solve. I'm closing this (again) in favor of #8857 |
|
How does an application set exdata on what |
There is currently no ... and this leads me to another thing: this discussion is off topic here, especially since this PR is closed. I suggest you raise an issue about |
|
Excellent point! See #8863 |
4 participants
With multiple library contexts, it's possible that the same module
runs its init and teardown functions multiple times during its
lifetime. This would normally not cause much trouble, except that our
FIPS module is a bit special and will include code that handles a
library context internally, just to be able to make internal EVP
calls.
This means that the provider init function will potentially be called
multiple times, and the teardown as well, and those may do
initializations and deinitializations, including the ex_data cleanup.
This may mean that the ex_data lock gets freed too early (on first
cleanup instead of last).
To ensure that the ex_data lock isn't freed before its last use, we
associate a reference counter with it, which gets incremented for
everytime there's an attempt to create that lock, and gets decremented
when there's an attempt to clean it up, thereby ensuring that
construction will only happen on first init and destruction on last
deinit.
Related to #8728