| Message ID | alpine.DEB.2.11.1611241400550.31044@cone.martin.st |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, Nov 24, 2016 at 02:04:03PM +0200, Martin Storsjö wrote: > On Thu, 24 Nov 2016, Diego Biurrun wrote: > > >On Wed, Nov 23, 2016 at 11:35:08PM +0200, Martin Storsjö wrote: > >>The rtmpdh code can use crypto libraries which may require > >>a process global init. (gcrypt is one of the libraries > >>where the rtmpdh test code can fail if global init hasn't been > >>done, depending on gcrypt version.) > >>--- > >> libavformat/tests/rtmpdh.c | 2 ++ > >> 1 file changed, 2 insertions(+) > > > >As you predicted, this does not fail my particular test case. > > does not fix, I presume Yes. > To fix that, you could just as an experiment try this: > > diff --git a/libavformat/tls_gnutls.c b/libavformat/tls_gnutls.c > index 3e29a45..abc9e69 100644 > --- a/libavformat/tls_gnutls.c > +++ b/libavformat/tls_gnutls.c > @@ -35,7 +35,8 @@ > #include "libavutil/opt.h" > #include "libavutil/parseutils.h" > > -#if HAVE_THREADS && GNUTLS_VERSION_NUMBER <= 0x020b00 > +#if CONFIG_GCRYPT > #include <gcrypt.h> > #include "libavutil/thread.h" > GCRY_THREAD_OPTION_PTHREAD_IMPL; > @@ -52,10 +53,10 @@ typedef struct TLSContext { > void ff_gnutls_init(void) > { > avpriv_lock_avformat(); > -#if HAVE_THREADS && GNUTLS_VERSION_NUMBER < 0x020b00 > +#if CONFIG_GCRYPT > if (gcry_control(GCRYCTL_ANY_INITIALIZATION_P) == 0) > gcry_control(GCRYCTL_SET_THREAD_CBS, &gcry_threads_pthread); > #endif > gnutls_global_init(); > avpriv_unlock_avformat(); > } That does the trick, please push :) > But I'm quite ok with dropping gcrypt support altogether. I'll send a patch later. > >>--- a/libavformat/tests/rtmpdh.c > >>+++ b/libavformat/tests/rtmpdh.c > >>@@ -150,6 +151,7 @@ fail: > >> > >> int main(void) > >> { > >>+ avformat_network_init(); > >> if (test_random_shared_secret() < 0) > >> return 1; > >> if (test_ref_data() < 0) > > > >LGTM > > Pushed to master > > >Please push to the 12 branch as well. > > Will try to remember to do that later, I'm short on time right now. Or feel > free to do that for me if you beat me to it. I'll queue it later. Diego
On Thu, 24 Nov 2016, Diego Biurrun wrote: >> To fix that, you could just as an experiment try this: >> >> diff --git a/libavformat/tls_gnutls.c b/libavformat/tls_gnutls.c >> index 3e29a45..abc9e69 100644 >> --- a/libavformat/tls_gnutls.c >> +++ b/libavformat/tls_gnutls.c >> @@ -35,7 +35,8 @@ >> #include "libavutil/opt.h" >> #include "libavutil/parseutils.h" >> >> -#if HAVE_THREADS && GNUTLS_VERSION_NUMBER <= 0x020b00 >> +#if CONFIG_GCRYPT >> #include <gcrypt.h> >> #include "libavutil/thread.h" >> GCRY_THREAD_OPTION_PTHREAD_IMPL; >> @@ -52,10 +53,10 @@ typedef struct TLSContext { >> void ff_gnutls_init(void) >> { >> avpriv_lock_avformat(); >> -#if HAVE_THREADS && GNUTLS_VERSION_NUMBER < 0x020b00 >> +#if CONFIG_GCRYPT >> if (gcry_control(GCRYCTL_ANY_INITIALIZATION_P) == 0) >> gcry_control(GCRYCTL_SET_THREAD_CBS, &gcry_threads_pthread); >> #endif >> gnutls_global_init(); >> avpriv_unlock_avformat(); >> } > > That does the trick, please push :) No, since this fix is not appropriate. It's not quite straightforward though. Notice how we don't have a separate --enable-gcrypt or --enable-gmp; this is kinda intentional. You're not supposed to use gcrypt because you want to, only if this happens to be the backend that your gnutls backend uses. Now the existing code is the correct (AFAIK) way of initializing gnutls for use in a threadsafe way; in older versions when gcrypt was the only available backend, the caller had to do it manually. In preparation for making gnutls use a different backend, this was folded into gnutls_global_init, and in newer gnutls versions, nettle/hogweed/gmp is the only backend. (And they don't need the global init.) If we'd consider gcrypt and gmp features that can be enabled/disabled separately from gnutls, we'd need to have some sort of init like this somewhere, but tls_gnutls.c surely isn't the right spot for it in that case. Prior to 57cde2b180fcec0eaf60aad65f436ab6420546f5, we had this init in network.c, where it was done if gnutls and gcrypt were enabled. But after that commit, this init actually only gets done if the tls protocol is enabled. This was pointed out in review IIRC, but the cleanup achieved is worth more than the fact that a build with --enable-gnutls --disable-protocol=tls won't init gnutls/gcrypt correctly for the rtmpdh code. To fix this correctly, we should have a separate, similar init for the crypto backends used by the rtmpdh code, not related to the tls protocols (called from within the same avformat_network_init). So far, this have felt like too much of a corner case to need to be cared about though. If we drop gcrypt, the issue goes away even more, since gmp doesn't need any global init. The libcrypt half of OpenSSL does need some sort of init in order to be threadsafe (but it doesn't fail if not inited, like some versions of gcrypt). So in such a setup (--enable-openssl --disable-protocol=tls), the rtmpdh code won't be threadsafe. // Martin
diff --git a/libavformat/tls_gnutls.c b/libavformat/tls_gnutls.c index 3e29a45..abc9e69 100644 --- a/libavformat/tls_gnutls.c +++ b/libavformat/tls_gnutls.c @@ -35,7 +35,8 @@ #include "libavutil/opt.h" #include "libavutil/parseutils.h" -#if HAVE_THREADS && GNUTLS_VERSION_NUMBER <= 0x020b00 +#if CONFIG_GCRYPT #include <gcrypt.h> #include "libavutil/thread.h" GCRY_THREAD_OPTION_PTHREAD_IMPL; @@ -52,10 +53,10 @@ typedef struct TLSContext {