Re: [FFmpeg-devel] [PATCH] rtmpdh: Initialize gcrypt before using it

2016-01-12 Thread Ricardo Constantino 
On 12 January 2016 at 05:05, Timothy Gu  wrote:

> Are you sure you want to initialize libgcrypt unconditionally as you are
> doing here?
>

I don't think it's unconditional. It only tries to initialize gcrypt if
it's not been initialized already.

>+if (!gcry_control(GCRYCTL_INITIALIZATION_FINISHED_P)) { \

But I'm not knowledgeable with C enough to argument that.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] rtmpdh: Initialize gcrypt before using it

2016-01-11 Thread Michael Niedermayer 
On Sun, Jan 10, 2016 at 10:04:34PM +, Ricardo Constantino wrote:
> On 10 January 2016 at 19:54, Michael Niedermayer 
> wrote:
> 
> >
> > please explain in the commit message why secmem is disabled
> >
> >

>  rtmpdh.c |   11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> a340654ed5741348282a81f4d1065f85b26896be  
> 0001-rtmpdh-Initialize-gcrypt-before-using-it.patch
> From 0225f408ef437e02d1dd5fc8e925e61120d56031 Mon Sep 17 00:00:00 2001
> From: Ricardo Constantino 
> Date: Tue, 29 Dec 2015 21:40:14 +
> Subject: [PATCH] rtmpdh: Initialize gcrypt before using it
> 
> Either disabling or init'ing secure memory is required after the use
> of gcry_check_version. From a look at the functions rtmpdh uses, I
> noticed none require the use of secure memory, so we disable it [1][2].
> 
> This resolves some errors returned by rtmpdh code with uninitialized
> gcrypt, especifically:
> Fatal: failed to create the RNG lock: Invalid argument
> FATAL: failed to acquire the FSM lock in libgrypt: Invalid argument
> 
> Version "1.5.4" was arbitrarily chosen. An older version probably works
> as well, but I couldn't compile older versions to test on my machine.
> 
> [1]
> https://gnupg.org/documentation/manuals/gcrypt/Initializing-the-library.html
> [2]
> https://www.gnupg.org/documentation/manuals/gcrypt/Controlling-the-library.html
> 
> Signed-off-by: Ricardo Constantino 
> ---
>  libavformat/rtmpdh.c | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

applied

thanks

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] rtmpdh: Initialize gcrypt before using it

2016-01-11 Thread Timothy Gu 
On Sun, Jan 10, 2016 at 10:04:34PM +, Ricardo Constantino wrote:

> Either disabling or init'ing secure memory is required after the use
> of gcry_check_version. From a look at the functions rtmpdh uses, I
> noticed none require the use of secure memory, so we disable it [1][2].
> 
> This resolves some errors returned by rtmpdh code with uninitialized
> gcrypt, especifically:
> Fatal: failed to create the RNG lock: Invalid argument
> FATAL: failed to acquire the FSM lock in libgrypt: Invalid argument
> 
> Version "1.5.4" was arbitrarily chosen. An older version probably works
> as well, but I couldn't compile older versions to test on my machine.
> 
> [1]
> https://gnupg.org/documentation/manuals/gcrypt/Initializing-the-library.html
> [2]
> https://www.gnupg.org/documentation/manuals/gcrypt/Controlling-the-library.html

Sorry for being late to the party, but [1] above says:

> It is important that these initialization steps are not done by a
> library but by the actual application. A library using Libgcrypt might
> want to check for finished initialization

Are you sure you want to initialize libgcrypt unconditionally as you are
doing here?

Timothy
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] rtmpdh: Initialize gcrypt before using it

2016-01-10 Thread Michael Niedermayer 
On Sun, Jan 10, 2016 at 07:34:27PM +, Ricardo Constantino wrote:
> Tried with 1.5.4 with the link on the first email and another one from
> Crunchyroll and it worked.
> Tried compiling 1.5.0 but didn't work (on MinGW).

>  rtmpdh.c |   11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 7a699f49fd1b4a33f4533702578d8ddd5b6ff7ad  
> 0001-rtmpdh-Initialize-gcrypt-before-using-it.patch
> From 821a9f99d04270a9c76c2c9ca897b1ace4227237 Mon Sep 17 00:00:00 2001
> From: Ricardo Constantino 
> Date: Tue, 29 Dec 2015 21:40:14 +
> Subject: [PATCH] rtmpdh: Initialize gcrypt before using it
> 
> Signed-off-by: Ricardo Constantino 
> ---
>  libavformat/rtmpdh.c | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/rtmpdh.c b/libavformat/rtmpdh.c
> index 91b1349..42ad72c 100644
> --- a/libavformat/rtmpdh.c
> +++ b/libavformat/rtmpdh.c
> @@ -97,7 +97,16 @@
>  mpz_fdiv_r_2exp(bn, bn, num_bits);\
>  } while (0)
>  #elif CONFIG_GCRYPT
> -#define bn_new(bn)  bn = gcry_mpi_new(1)
> +#define bn_new(bn)  \
> +do {\
> +if (!gcry_control(GCRYCTL_INITIALIZATION_FINISHED_P)) { \
> +if (!gcry_check_version("1.5.4"))   \
> +return AVERROR(EINVAL); \

> +gcry_control(GCRYCTL_DISABLE_SECMEM, 0);\

please explain in the commit message why secmem is disabled

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] rtmpdh: Initialize gcrypt before using it

2016-01-10 Thread Ricardo Constantino 
Tried with 1.5.4 with the link on the first email and another one from
Crunchyroll and it worked.
Tried compiling 1.5.0 but didn't work (on MinGW).


0001-rtmpdh-Initialize-gcrypt-before-using-it.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] rtmpdh: Initialize gcrypt before using it

2016-01-10 Thread Ricardo Constantino 
On 10 January 2016 at 19:54, Michael Niedermayer 
wrote:

>
> please explain in the commit message why secmem is disabled
>
>


0001-rtmpdh-Initialize-gcrypt-before-using-it.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] rtmpdh: Initialize gcrypt before using it

2016-01-09 Thread Ricardo Constantino 
Ping
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] rtmpdh: Initialize gcrypt before using it

2016-01-09 Thread Ricardo Constantino 
On 10 January 2016 at 00:17, Ricardo Constantino  wrote:

> On 9 January 2016 at 23:32, Michael Niedermayer 
> wrote:
>
>>
>> we should pass the version number we need i think
>> see
>>
>> https://quickgit.kde.org/?p=libktorrent.git=commit=a23bafe9191e0dcbd5ae75335fd5d8e49bfda9ed
>>
>>
> I wouldn't know the minimum version FFmpeg needs.
> Wouldn't it be better to leave as NULL then?
>
On a glance of libgcrypt's NEWS I can't find any mention of most functions
used in rtmpdh.c, except for gcry_mpi_{print,scan} which just had:
>gcry_mpi_scanCHANGED: New argument to separate in/out args.
>gcry_mpi_printCHANGED: Ditto.
in 1.1.42 (2003)
and added support for negative numbers in 1.6.0.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] rtmpdh: Initialize gcrypt before using it

2016-01-09 Thread Michael Niedermayer 
On Mon, Jan 04, 2016 at 02:39:41AM +, Ricardo Constantino wrote:
> On 3 January 2016 at 23:13, Jan Ekstrom  wrote:
> 
> > Hi,
> >
> > In general looks good, although it might look a bit weird for someone
> > as usually libraries have initialization functions called like that.
> > That said, this is what
> >
> > https://gnupg.org/documentation/manuals/gcrypt/Initializing-the-library.html
> > recommends.
> >
> > My only comment would be that we might want to set the parameter to
> > GCRYPT_VERSION instead of NULL, as we most probably care if the
> > library loaded matches our version (unless these versions change even
> > if API doesn't change).
> >
> 
> This is where it shows my ignorance but I don't know which of these is the
> best way of dealing with that.
> I tested all three and they all work with the linked sample.

we should pass the version number we need i think
see
https://quickgit.kde.org/?p=libktorrent.git=commit=a23bafe9191e0dcbd5ae75335fd5d8e49bfda9ed

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] rtmpdh: Initialize gcrypt before using it

2016-01-09 Thread Ricardo Constantino 
On 9 January 2016 at 23:32, Michael Niedermayer 
wrote:

>
> we should pass the version number we need i think
> see
>
> https://quickgit.kde.org/?p=libktorrent.git=commit=a23bafe9191e0dcbd5ae75335fd5d8e49bfda9ed
>
>
I wouldn't know the minimum version FFmpeg needs.
Wouldn't it be better to leave as NULL then?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] rtmpdh: Initialize gcrypt before using it

2016-01-03 Thread Ricardo Constantino 
On 3 January 2016 at 23:13, Jan Ekstrom  wrote:

> Hi,
>
> In general looks good, although it might look a bit weird for someone
> as usually libraries have initialization functions called like that.
> That said, this is what
>
> https://gnupg.org/documentation/manuals/gcrypt/Initializing-the-library.html
> recommends.
>
> My only comment would be that we might want to set the parameter to
> GCRYPT_VERSION instead of NULL, as we most probably care if the
> library loaded matches our version (unless these versions change even
> if API doesn't change).
>

This is where it shows my ignorance but I don't know which of these is the
best way of dealing with that.
I tested all three and they all work with the linked sample.


0001-rtmpdh-Initialize-gcrypt-before-using-it-v1.patch
Description: Binary data


0001-rtmpdh-Initialize-gcrypt-before-using-it-v2.patch
Description: Binary data


0001-rtmpdh-Initialize-gcrypt-before-using-it-v3.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] rtmpdh: Initialize gcrypt before using it

2016-01-03 Thread Jan Ekstrom 
Hi,

In general looks good, although it might look a bit weird for someone
as usually libraries have initialization functions called like that.
That said, this is what
https://gnupg.org/documentation/manuals/gcrypt/Initializing-the-library.html
recommends.

My only comment would be that we might want to set the parameter to
GCRYPT_VERSION instead of NULL, as we most probably care if the
library loaded matches our version (unless these versions change even
if API doesn't change).

Best regards,
Jan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel