| Message ID | 20190117085715.44726-1-rodger.combs@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Rodger Combs (12019-01-17): > --- > libavformat/tls_openssl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/tls_openssl.c b/libavformat/tls_openssl.c > index 7ae71bdaf3..faa5b8636e 100644 > --- a/libavformat/tls_openssl.c > +++ b/libavformat/tls_openssl.c > @@ -102,7 +102,7 @@ void ff_openssl_deinit(void) > openssl_init--; > if (!openssl_init) { > #if HAVE_THREADS > - if (CRYPTO_get_locking_callback() == openssl_lock) { > + if (CRYPTO_get_locking_callback() == &openssl_lock) { Using the & operator on a function seems strange. What warnings is it supposed to fix, and why? > int i; > CRYPTO_set_locking_callback(NULL); > for (i = 0; i < CRYPTO_num_locks(); i++) Regards,
> On Jan 17, 2019, at 03:12, Nicolas George <george@nsup.org> wrote: > > Signed PGP part > Rodger Combs (12019-01-17): >> --- >> libavformat/tls_openssl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/tls_openssl.c b/libavformat/tls_openssl.c >> index 7ae71bdaf3..faa5b8636e 100644 >> --- a/libavformat/tls_openssl.c >> +++ b/libavformat/tls_openssl.c >> @@ -102,7 +102,7 @@ void ff_openssl_deinit(void) >> openssl_init--; >> if (!openssl_init) { >> #if HAVE_THREADS > >> - if (CRYPTO_get_locking_callback() == openssl_lock) { >> + if (CRYPTO_get_locking_callback() == &openssl_lock) { > > Using the & operator on a function seems strange. What warnings is it > supposed to fix, and why? CRYPTO_get_locking_callback is a macro returning NULL on 1.1 and later. This triggers -Wtautological-pointer-compare ("comparison of function 'openssl_lock' equal to a null pointer is always false"), which suggests "prefix with the address-of operator to silence this warning", so I did that. We could alternately wrap this code in an OpenSSL version check, but this seemed easier. > >> int i; >> CRYPTO_set_locking_callback(NULL); >> for (i = 0; i < CRYPTO_num_locks(); i++) > > Regards, > > -- > Nicolas George > >
Rodger Combs (12019-01-17): > CRYPTO_get_locking_callback is a macro returning NULL on 1.1 and > later. This triggers -Wtautological-pointer-compare ("comparison of > function 'openssl_lock' equal to a null pointer is always false"), > which suggests "prefix with the address-of operator to silence this > warning", so I did that. We could alternately wrap this code in an > OpenSSL version check, but this seemed easier. Urgh. Somebody is to blame for that ugliness, but certainly not you. If it gets in, I suggest you add a comment to explain that. But this is worrying: elsewhere in the code, CRYPTO_get_locking_callback() is tested and an array is mallocated if it is null: that looks like a leak. I think part of that code needs to be changed to test the array itself instead of the callbacks. Regards,
diff --git a/libavformat/tls_openssl.c b/libavformat/tls_openssl.c index 7ae71bdaf3..faa5b8636e 100644 --- a/libavformat/tls_openssl.c +++ b/libavformat/tls_openssl.c @@ -102,7 +102,7 @@ void ff_openssl_deinit(void) openssl_init--; if (!openssl_init) { #if HAVE_THREADS - if (CRYPTO_get_locking_callback() == openssl_lock) { + if (CRYPTO_get_locking_callback() == &openssl_lock) { int i; CRYPTO_set_locking_callback(NULL); for (i = 0; i < CRYPTO_num_locks(); i++)