[FFmpeg-devel] [PATCH 0/3] Fix invalid frees, segfaults and memory leaks in avcodec/fft wrappers

[FFmpeg-devel] [PATCH 0/3] Fix invalid frees, segfaults and memory leaks in avcodec/fft wrappers

[Sebastian Ramacher]

The wrappers in avcodec/fft which were introduced in 6.1 may lead to invalid frees, segfaults and
memory leaks. Consider the following example program:

#include <libavcodec/avfft.h>

int main() {
  FFTContext* fft = av_fft_init(11, 0);
  av_fft_end(fft);

  FFTContext* mdct = av_mdct_init(11, 0, 1.0);
  av_mdct_end(mdct);

  mdct = av_mdct_init(11, 1, 1.0);
  av_mdct_end(mdct);

  RDFTContext* rdft = av_rdft_init(11, DFT_R2C);
  av_rdft_end(rdft);

  DCTContext* dct = av_dct_init(11, DCT_II);
  av_dct_end(dct);
}

When executed under valgrind, one obtains:

==2300086== Memcheck, a memory error detector
==2300086== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==2300086== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
==2300086== Command: ./a.out
==2300086== 
==2300086== Conditional jump or move depends on uninitialised value(s)
==2300086==    at 0x5FB6CBE: av_tx_uninit (in /usr/lib/x86_64-linux-gnu/libavutil.so.58.29.100)
==2300086==    by 0x490B3AA: av_fft_end (in /usr/lib/x86_64-linux-gnu/libavcodec.so.60.31.102)
==2300086==    by 0x1090D7: main (test.c:5)
==2300086==  Uninitialised value was created by a heap allocation
==2300086==    at 0x4845990: memalign (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2300086==    by 0x4845AED: posix_memalign (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2300086==    by 0x5FFAC14: av_malloc (in /usr/lib/x86_64-linux-gnu/libavutil.so.58.29.100)
==2300086==    by 0x4A4B4A5: av_fft_init (in /usr/lib/x86_64-linux-gnu/libavcodec.so.60.31.102)
==2300086==    by 0x1090CF: main (test.c:4)
==2300086== 
==2300086== Conditional jump or move depends on uninitialised value(s)
==2300086==    at 0x4843131: free (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2300086==    by 0x490B4A5: av_dct_end (in /usr/lib/x86_64-linux-gnu/libavcodec.so.60.31.102)
==2300086==    by 0x10913A: main (test.c:17)
==2300086==  Uninitialised value was created by a heap allocation
==2300086==    at 0x4845990: memalign (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2300086==    by 0x4845AED: posix_memalign (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2300086==    by 0x5FFAC14: av_malloc (in /usr/lib/x86_64-linux-gnu/libavutil.so.58.29.100)
==2300086==    by 0x4A4B96F: av_dct_init (in /usr/lib/x86_64-linux-gnu/libavcodec.so.60.31.102)
==2300086==    by 0x109132: main (test.c:16)
==2300086== 
==2300086== 
==2300086== HEAP SUMMARY:
==2300086==     in use at exit: 66,528 bytes in 270 blocks
==2300086==   total heap usage: 1,353 allocs, 1,083 frees, 386,566 bytes allocated
==2300086== 
==2300086== 8,064 (640 direct, 7,424 indirect) bytes in 1 blocks are definitely lost in loss record 247 of 249
==2300086==    at 0x4845990: memalign (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2300086==    by 0x4845AED: posix_memalign (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2300086==    by 0x5FFAC14: av_malloc (in /usr/lib/x86_64-linux-gnu/libavutil.so.58.29.100)
==2300086==    by 0x5FFAF80: av_mallocz (in /usr/lib/x86_64-linux-gnu/libavutil.so.58.29.100)
==2300086==    by 0x5FB732D: ??? (in /usr/lib/x86_64-linux-gnu/libavutil.so.58.29.100)
==2300086==    by 0x5FB7616: av_tx_init (in /usr/lib/x86_64-linux-gnu/libavutil.so.58.29.100)
==2300086==    by 0x4A4B678: av_mdct_init (in /usr/lib/x86_64-linux-gnu/libavcodec.so.60.31.102)
==2300086==    by 0x10910A: main (test.c:10)
==2300086== 
==2300086== 8,192 bytes in 1 blocks are possibly lost in loss record 248 of 249
==2300086==    at 0x4845990: memalign (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2300086==    by 0x4845AED: posix_memalign (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2300086==    by 0x5FFAC14: av_malloc (in /usr/lib/x86_64-linux-gnu/libavutil.so.58.29.100)
==2300086==    by 0x6030F60: ??? (in /usr/lib/x86_64-linux-gnu/libavutil.so.58.29.100)
==2300086==    by 0x5FBC968: ??? (in /usr/lib/x86_64-linux-gnu/libavutil.so.58.29.100)
==2300086==    by 0x5FB73C8: ??? (in /usr/lib/x86_64-linux-gnu/libavutil.so.58.29.100)
==2300086==    by 0x5FB73C8: ??? (in /usr/lib/x86_64-linux-gnu/libavutil.so.58.29.100)
==2300086==    by 0x5FB7616: av_tx_init (in /usr/lib/x86_64-linux-gnu/libavutil.so.58.29.100)
==2300086==    by 0x4A4B678: av_mdct_init (in /usr/lib/x86_64-linux-gnu/libavcodec.so.60.31.102)
==2300086==    by 0x10910A: main (test.c:10)
==2300086== 
==2300086== LEAK SUMMARY:
==2300086==    definitely lost: 640 bytes in 1 blocks
==2300086==    indirectly lost: 7,424 bytes in 4 blocks
==2300086==      possibly lost: 8,192 bytes in 1 blocks
==2300086==    still reachable: 48,256 bytes in 243 blocks
==2300086==         suppressed: 0 bytes in 0 blocks
==2300086== Reachable blocks (those to which a pointer was found) are not shown.
==2300086== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==2300086== 
==2300086== For lists of detected and suppressed errors, rerun with: -s
==2300086== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 0 from 0)

This patch series fixes the above issues. The initial issue in av_fft_end was discuvered via the
test suite of r-cran-av.

Sebastian Ramacher (3):
  avcodec/fft: Do not uninit never initialized ctx2
  avcodec/fft: Set potentially unused wrapper variables to avoid invalid
    free/uninit
  avcoded/fft: Fix memory leak if ctx2 is used

 libavcodec/avfft.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

-- 
2.42.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".

[FFmpeg-devel] [PATCH 1/3] avcodec/fft: Do not uninit never initialized ctx2

[Sebastian Ramacher]

---
 libavcodec/avfft.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libavcodec/avfft.c b/libavcodec/avfft.c
index fb635abfff..93203228c2 100644
--- a/libavcodec/avfft.c
+++ b/libavcodec/avfft.c
@@ -76,7 +76,6 @@ av_cold void av_fft_end(FFTContext *s)
     if (s) {
         AVTXWrapper *w = (AVTXWrapper *)s;
         av_tx_uninit(&w->ctx);
-        av_tx_uninit(&w->ctx2);
         av_free(w);
     }
 }
-- 
2.42.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".

[FFmpeg-devel] [PATCH 2/3] avcodec/fft: Set potentially unused wrapper variables to avoid invalid free/uninit

[Sebastian Ramacher]

---
 libavcodec/avfft.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libavcodec/avfft.c b/libavcodec/avfft.c
index 93203228c2..813b6d61a1 100644
--- a/libavcodec/avfft.c
+++ b/libavcodec/avfft.c
@@ -102,7 +102,8 @@ FFTContext *av_mdct_init(int nbits, int inverse, double scale)
             av_free(s);
             return NULL;
         }
-    }
+    } else
+        s->ctx2 = NULL;
 
     return (FFTContext *)s;
 }
@@ -220,7 +221,8 @@ DCTContext *av_dct_init(int nbits, enum DCTTransformType inverse)
             av_free(s);
             return NULL;
         }
-    }
+    } else
+        s->tmp = NULL;
 
     return (DCTContext *)s;
 }
-- 
2.42.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".

[FFmpeg-devel] [PATCH 3/3] avcoded/fft: Fix memory leak if ctx2 is used

[Sebastian Ramacher]

---
 libavcodec/avfft.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavcodec/avfft.c b/libavcodec/avfft.c
index 813b6d61a1..1b8d6f76c3 100644
--- a/libavcodec/avfft.c
+++ b/libavcodec/avfft.c
@@ -130,6 +130,7 @@ av_cold void av_mdct_end(FFTContext *s)
 {
     if (s) {
         AVTXWrapper *w = (AVTXWrapper *)s;
+        av_tx_uninit(&w->ctx2);
         av_tx_uninit(&w->ctx);
         av_free(w);
     }
-- 
2.42.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 2/3] avcodec/fft: Set potentially unused wrapper variables to avoid invalid free/uninit

[James Almer]

On 11/12/2023 12:11 PM, Sebastian Ramacher wrote:
> ---
>   libavcodec/avfft.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/avfft.c b/libavcodec/avfft.c
> index 93203228c2..813b6d61a1 100644
> --- a/libavcodec/avfft.c
> +++ b/libavcodec/avfft.c
> @@ -102,7 +102,8 @@ FFTContext *av_mdct_init(int nbits, int inverse, double scale)
>               av_free(s);
>               return NULL;
>           }
> -    }
> +    } else
> +        s->ctx2 = NULL;
>   
>       return (FFTContext *)s;
>   }
> @@ -220,7 +221,8 @@ DCTContext *av_dct_init(int nbits, enum DCTTransformType inverse)
>               av_free(s);
>               return NULL;
>           }
> -    }
> +    } else
> +        s->tmp = NULL;
>   
>       return (DCTContext *)s;
>   }

IMO just allocate the AVTXWrapper with av_mallocz() instead.
_______________________________________________
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".

[FFmpeg-devel] [PATCHv2 1/2] avcodec/fft: Use av_mallocz to avoid invalid free/uninit

[Sebastian Ramacher]

---
 libavcodec/avfft.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavcodec/avfft.c b/libavcodec/avfft.c
index fb635abfff..3ef076d222 100644
--- a/libavcodec/avfft.c
+++ b/libavcodec/avfft.c
@@ -46,7 +46,7 @@ FFTContext *av_fft_init(int nbits, int inverse)
 {
     int ret;
     float scale = 1.0f;
-    AVTXWrapper *s = av_malloc(sizeof(*s));
+    AVTXWrapper *s = av_mallocz(sizeof(*s));
     if (!s)
         return NULL;
 
@@ -85,7 +85,7 @@ FFTContext *av_mdct_init(int nbits, int inverse, double scale)
 {
     int ret;
     float scale_f = scale;
-    AVTXWrapper *s = av_malloc(sizeof(*s));
+    AVTXWrapper *s = av_mallocz(sizeof(*s));
     if (!s)
         return NULL;
 
@@ -146,7 +146,7 @@ RDFTContext *av_rdft_init(int nbits, enum RDFTransformType trans)
     if (trans != IDFT_C2R && trans != DFT_R2C)
         return NULL;
 
-    s = av_malloc(sizeof(*s));
+    s = av_mallocz(sizeof(*s));
     if (!s)
         return NULL;
 
@@ -199,7 +199,7 @@ DCTContext *av_dct_init(int nbits, enum DCTTransformType inverse)
         [DST_I] = AV_TX_FLOAT_DST_I,
     };
 
-    AVTXWrapper *s = av_malloc(sizeof(*s));
+    AVTXWrapper *s = av_mallocz(sizeof(*s));
     if (!s)
         return NULL;
 
-- 
2.42.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".

[FFmpeg-devel] [PATCHv2 2/2] avcoded/fft: Fix memory leak if ctx2 is used

[Sebastian Ramacher]

---
 libavcodec/avfft.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavcodec/avfft.c b/libavcodec/avfft.c
index 3ef076d222..999b5ed79a 100644
--- a/libavcodec/avfft.c
+++ b/libavcodec/avfft.c
@@ -130,6 +130,7 @@ av_cold void av_mdct_end(FFTContext *s)
 {
     if (s) {
         AVTXWrapper *w = (AVTXWrapper *)s;
+        av_tx_uninit(&w->ctx2);
         av_tx_uninit(&w->ctx);
         av_free(w);
     }
-- 
2.42.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".