crypto/provider_core.c: correct definition of lock #8477
Conversation
| @@ -208,7 +208,7 @@ void ossl_provider_free(OSSL_PROVIDER *prov) | |||
| int ref = 0; | |||
|
|
|||
| #ifndef HAVE_ATOMICS | |||
| CRYPTO_DOWN_REF(&prov->refcnt, &ref, provider_lock); | |||
| CRYPTO_DOWN_REF(&prov->refcnt, &ref, prov->refcnt_lock); | |||
There was a problem hiding this comment.
refcnt_lock appears to be NULL?
There was a problem hiding this comment.
Argh...
The refcount header doesn't make it easy to build without atomics 😕
That needs to change.
There was a problem hiding this comment.
I do wonder why we're using HAVE_ATOMICS here at all. We could just create the lock unconditionally. If HAVE_ATOMICS is defined then it won't be used by the CRYPTO_DOWN_REF implementation...but so what?
There was a problem hiding this comment.
'til some code sanitizer notices and starts complaining about it
There was a problem hiding this comment.
It's what we do everywhere else. This is the only file in the whole repo that actually uses the HAVE_ATOMICS define.
There was a problem hiding this comment.
Okie. I've now ripped out most of those ifdefs. I will refuse to remove those around CRYPTO_THREAD_lock_new and CRYPTO_THREAD_lock_free, though. That would be stoopid waste of resources (and if we are wasteful elsewhere, I think guards should be added)
| @@ -231,6 +224,9 @@ void ossl_provider_free(OSSL_PROVIDER *prov) | |||
| if (ref == 0) { | |||
| DSO_free(prov->module); | |||
| OPENSSL_free(prov->name); | |||
| #ifndef HAVE_ATOMICS | |||
| CRYPTO_THREAD_lock_free(prov->refcnt_lock); | |||
| #endif | |||
There was a problem hiding this comment.
You could also make this unconditional
There was a problem hiding this comment.
I'll have a closer look at the HAVE_ATOMICS stuff. We could do it better
There was a problem hiding this comment.
I planned on producing a macro or two that do the necessary conditioning for us.
It shouldn't be hard to make this a lot easier.
|
Merged. 085bef9 crypto/provider_core.c: correct definition and use of lock |
|
Should CRYPTO_LOCK be used in the same in all API's no matter what ifdef's are used? Isn't that the point of the abstraction? |
We ended up doing that here as well. The only thing I still guard is the creation of the lock. There's no point creating it if it won't be used... |
|
Oops, I misread the diff. Yes, you're completely right and I wasted time. |
Fixes #8476