[FFmpeg-devel] [PATCH] avformat/ffrtmpcrypt: Fix int-conversion warning

[FFmpeg-devel] [PATCH] avformat/ffrtmpcrypt: Fix int-conversion warning

[Frank Plowman]

The gcrypt definition of `bn_new` used to use `AVERROR`, however it is
called in `dh_generate_key` and `ff_dh_init` which return pointers. As a
result, compiling with gcrypt and the ffrtmpcrypt protocol resulted in an
int-conversion warning. GCC 14 may upgrade these to errors [1].

This patch fixes the problem by changing the macro to remove `AVERROR`
and instead set `bn` to null if the allocation fails. This is the
behaviour of all the other `bn_new` implementations and so the result is
already checked at all the callsites. AFAICT, this should be the only
change needed to get ffmpeg off Fedora's naughty list of projects with
warnings which may be upgraded to errors in GCC 14 [2].

[1]: https://gcc.gnu.org/pipermail/gcc/2023-May/241264.html
[2]: https://www.mail-archive.com/devel@lists.fedoraproject.org/msg196024.html

Signed-off-by: Frank Plowman <post@frankplowman.com>
---
 libavformat/rtmpdh.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/libavformat/rtmpdh.c b/libavformat/rtmpdh.c
index 5ddae537a1..6a6c2ccd87 100644
--- a/libavformat/rtmpdh.c
+++ b/libavformat/rtmpdh.c
@@ -113,15 +113,18 @@ static int bn_modexp(FFBigNum bn, FFBigNum y, FFBigNum q, FFBigNum p)
     return 0;
 }
 #elif CONFIG_GCRYPT
-#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);            \
-            gcry_control(GCRYCTL_INITIALIZATION_FINISHED, 0);   \
-        }                                                       \
-        bn = gcry_mpi_new(1);                                   \
+#define bn_new(bn)                                                \
+    do {                                                          \
+        if (!gcry_control(GCRYCTL_INITIALIZATION_FINISHED_P)) {   \
+            if (gcry_check_version("1.5.4")) {                    \
+                gcry_control(GCRYCTL_DISABLE_SECMEM, 0);          \
+                gcry_control(GCRYCTL_INITIALIZATION_FINISHED, 0); \
+            }                                                     \
+        }                                                         \
+        if (gcry_control(GCRYCTL_INITIALIZATION_FINISHED_P))      \
+            bn = gcry_mpi_new(1);                                 \
+        else                                                      \
+            bn = NULL;                                            \
     } while (0)
 #define bn_free(bn)                 gcry_mpi_release(bn)
 #define bn_set_word(bn, w)          gcry_mpi_set_ui(bn, w)
-- 
2.43.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/ffrtmpcrypt: Fix int-conversion warning

[Martin Storsjö]

Hi Frank,

On Fri, 22 Dec 2023, Frank Plowman wrote:

> The gcrypt definition of `bn_new` used to use `AVERROR`, however it is
> called in `dh_generate_key` and `ff_dh_init` which return pointers. As a
> result, compiling with gcrypt and the ffrtmpcrypt protocol resulted in an
> int-conversion warning. GCC 14 may upgrade these to errors [1].

(FWIW, the issue that bn_new was used in functions that don't return an 
error-signaling integer was present when this macro was added originally 
in d50b5d547f4070678c88aa095b5292c872e2c1dc to.)

The change LGTM, but the wording here is slightly confusing IMO. The 
problem isn't with using per se AVERROR, that's just a macro for 
generating suitable integers, the issue is more about the fact that we're 
returning from a macro, without knowing the actual context where the macro 
is invoked.

WDYT about this wording?

> The gcrypt definition of `bn_new` used to use the return statement on 
> errors, with an AVERROR return value, regardless of the signature of the 
> function where the macro is used - it is called in `dh_generate_key` and 
> `ff_dh_init` which return pointers. As a result, compiling with gcrypt 
> and the ffrtmpcrypt protocol resulted in an int-conversion warning. GCC 
> 14 may upgrade these to errors [1].

// Martin

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/ffrtmpcrypt: Fix int-conversion warning

[Frank Plowman]

Hi Martin,

Thanks for the review.

On 22/12/2023 12:15, Martin Storsjö wrote:
> The change LGTM, but the wording here is slightly confusing IMO. The 
> problem isn't with using per se AVERROR, that's just a macro for 
> generating suitable integers, the issue is more about the fact that 
> we're returning from a macro, without knowing the actual context where 
> the macro is invoked.
>
> WDYT about this wording?
>
>> The gcrypt definition of `bn_new` used to use the return statement on 
>> errors, with an AVERROR return value, regardless of the signature of 
>> the function where the macro is used - it is called in 
>> `dh_generate_key` and `ff_dh_init` which return pointers. As a 
>> result, compiling with gcrypt and the ffrtmpcrypt protocol resulted 
>> in an int-conversion warning. GCC 14 may upgrade these to errors [1].

Yeah this is better, I agree.

Cheers,
Frank

-- 
https://www.frankplowman.com/

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/ffrtmpcrypt: Fix int-conversion warning

[Martin Storsjö]

On Fri, 22 Dec 2023, Frank Plowman wrote:

> Hi Martin,
>
> Thanks for the review.
>
> On 22/12/2023 12:15, Martin Storsjö wrote:
>> The change LGTM, but the wording here is slightly confusing IMO. The 
>> problem isn't with using per se AVERROR, that's just a macro for generating 
>> suitable integers, the issue is more about the fact that we're returning 
>> from a macro, without knowing the actual context where the macro is 
>> invoked.
>> 
>> WDYT about this wording?
>> 
>>> The gcrypt definition of `bn_new` used to use the return statement on 
>>> errors, with an AVERROR return value, regardless of the signature of the 
>>> function where the macro is used - it is called in `dh_generate_key` and 
>>> `ff_dh_init` which return pointers. As a result, compiling with gcrypt and 
>>> the ffrtmpcrypt protocol resulted in an int-conversion warning. GCC 14 may 
>>> upgrade these to errors [1].
>
> Yeah this is better, I agree.

Pushed now, thanks for the patch!

// Martin
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".