rtmpdh: Do global initialization before running the test

Message ID alpine.DEB.2.11.1611241400550.31044@cone.martin.st
State Not Applicable
Headers show

Commit Message

Martin Storsjö Nov. 24, 2016, 12:04 p.m.
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

To fix that, you could just as an experiment try this:

  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();
  }


But I'm quite ok with dropping gcrypt support altogether.


>> --- a/libavformat/tests/rtmpdh.c
>> +++ b/libavformat/tests/rtmpdh.c
>> @@ -17,6 +17,7 @@
>>
>>  #include "libavformat/rtmpdh.c"
>> +#include "libavformat/avformat.h"
>
> nit: order

Fixed

>> @@ -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.

// Martin

Comments

Diego Biurrun Nov. 24, 2016, 12:26 p.m. | #1
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
Martin Storsjö Nov. 24, 2016, 7:49 p.m. | #2
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

Patch

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 {