diff mbox

[FFmpeg-devel,v2,5/5] avformat/chromaprint: Avoid null pointer dereference

Message ID 20191006054950.30374-5-andriy.gelman@gmail.com
State Withdrawn
Headers show

Commit Message

Andriy Gelman Oct. 6, 2019, 5:49 a.m. UTC
From: Andriy Gelman <andriy.gelman@gmail.com>

As of commit 21b2442f in the chromaprint library, selecting "-algorithm 2" via the ffmpeg cli creates a null pointer dereference. This can be replicated by:
./ffmpeg -f lavfi -i sine=d=20,asetnsamples=n=1000 -f chromaprint -algorithm 2 -

Until this issue is resolved, this commit makes ffmpeg output an error when
"-algorithm 2" is selected for chromaprint versions > 1.2.0.
---
 libavformat/chromaprint.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Andriy Gelman Oct. 6, 2019, 6 a.m. UTC | #1
On Sun, 06. Oct 01:49, Andriy Gelman wrote:
> From: Andriy Gelman <andriy.gelman@gmail.com>
> 
> As of commit 21b2442f in the chromaprint library, selecting "-algorithm 2" via the ffmpeg cli creates a null pointer dereference. This can be replicated by:
> ./ffmpeg -f lavfi -i sine=d=20,asetnsamples=n=1000 -f chromaprint -algorithm 2 -
> 
> Until this issue is resolved, this commit makes ffmpeg output an error when
> "-algorithm 2" is selected for chromaprint versions > 1.2.0.
> ---
>  libavformat/chromaprint.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/libavformat/chromaprint.c b/libavformat/chromaprint.c
> index faa92ca0db..3ecce3e08a 100644
> --- a/libavformat/chromaprint.c
> +++ b/libavformat/chromaprint.c
> @@ -70,6 +70,13 @@ static int write_header(AVFormatContext *s)
>          return AVERROR(ENOMEM);
>      }
>  
> +#if CPR_VERSION_INT > AV_VERSION_INT(1, 2, 0)
> +    if (cpr->algorithm == CHROMAPRINT_ALGORITHM_TEST3) {
> +        av_log(s, AV_LOG_ERROR, "Algorithm 2 cannot be used with chromaprint version > 1.2.0 because of a bug in the chromaprint library\n");
> +        goto fail;
> +    }
> +#endif
> +
>      if (cpr->silence_threshold != -1) {
>  #if CPR_VERSION_INT >= AV_VERSION_INT(0, 7, 0)
>          if (!chromaprint_set_option(cpr->ctx, "silence_threshold", cpr->silence_threshold)) {
> -- 
> 2.23.0
> 

The seg fault actually occurs in libavcodec/avfft.c in the
av_rdft_calc(RDFTContext *s, FFTSample *data) function, where chromaprint lib parses an unitialized context s=NULL. 

Is it worth submitting a patch where contexts are checked before dereferencing
in avfft.c? 

--
Andriy
Paul B Mahol Oct. 6, 2019, 11:56 a.m. UTC | #2
On 10/6/19, Andriy Gelman <andriy.gelman@gmail.com> wrote:
> On Sun, 06. Oct 01:49, Andriy Gelman wrote:
>> From: Andriy Gelman <andriy.gelman@gmail.com>
>>
>> As of commit 21b2442f in the chromaprint library, selecting "-algorithm 2"
>> via the ffmpeg cli creates a null pointer dereference. This can be
>> replicated by:
>> ./ffmpeg -f lavfi -i sine=d=20,asetnsamples=n=1000 -f chromaprint
>> -algorithm 2 -
>>
>> Until this issue is resolved, this commit makes ffmpeg output an error
>> when
>> "-algorithm 2" is selected for chromaprint versions > 1.2.0.
>> ---
>>  libavformat/chromaprint.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/libavformat/chromaprint.c b/libavformat/chromaprint.c
>> index faa92ca0db..3ecce3e08a 100644
>> --- a/libavformat/chromaprint.c
>> +++ b/libavformat/chromaprint.c
>> @@ -70,6 +70,13 @@ static int write_header(AVFormatContext *s)
>>          return AVERROR(ENOMEM);
>>      }
>>
>> +#if CPR_VERSION_INT > AV_VERSION_INT(1, 2, 0)
>> +    if (cpr->algorithm == CHROMAPRINT_ALGORITHM_TEST3) {
>> +        av_log(s, AV_LOG_ERROR, "Algorithm 2 cannot be used with
>> chromaprint version > 1.2.0 because of a bug in the chromaprint
>> library\n");
>> +        goto fail;
>> +    }
>> +#endif
>> +
>>      if (cpr->silence_threshold != -1) {
>>  #if CPR_VERSION_INT >= AV_VERSION_INT(0, 7, 0)
>>          if (!chromaprint_set_option(cpr->ctx, "silence_threshold",
>> cpr->silence_threshold)) {
>> --
>> 2.23.0
>>
>
> The seg fault actually occurs in libavcodec/avfft.c in the
> av_rdft_calc(RDFTContext *s, FFTSample *data) function, where chromaprint
> lib parses an unitialized context s=NULL.
>
> Is it worth submitting a patch where contexts are checked before
> dereferencing
> in avfft.c?

No, fix chromaprint instead, its very buggy.

>
> --
> Andriy
> _______________________________________________
> 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".
Andriy Gelman Oct. 6, 2019, 1:23 p.m. UTC | #3
On Sun, 06. Oct 13:56, Paul B Mahol wrote:
> On 10/6/19, Andriy Gelman <andriy.gelman@gmail.com> wrote:
> > On Sun, 06. Oct 01:49, Andriy Gelman wrote:
> >> From: Andriy Gelman <andriy.gelman@gmail.com>
> >>
> >> As of commit 21b2442f in the chromaprint library, selecting "-algorithm 2"
> >> via the ffmpeg cli creates a null pointer dereference. This can be
> >> replicated by:
> >> ./ffmpeg -f lavfi -i sine=d=20,asetnsamples=n=1000 -f chromaprint
> >> -algorithm 2 -
> >>
> >> Until this issue is resolved, this commit makes ffmpeg output an error
> >> when
> >> "-algorithm 2" is selected for chromaprint versions > 1.2.0.
> >> ---
> >>  libavformat/chromaprint.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/libavformat/chromaprint.c b/libavformat/chromaprint.c
> >> index faa92ca0db..3ecce3e08a 100644
> >> --- a/libavformat/chromaprint.c
> >> +++ b/libavformat/chromaprint.c
> >> @@ -70,6 +70,13 @@ static int write_header(AVFormatContext *s)
> >>          return AVERROR(ENOMEM);
> >>      }
> >>
> >> +#if CPR_VERSION_INT > AV_VERSION_INT(1, 2, 0)
> >> +    if (cpr->algorithm == CHROMAPRINT_ALGORITHM_TEST3) {
> >> +        av_log(s, AV_LOG_ERROR, "Algorithm 2 cannot be used with
> >> chromaprint version > 1.2.0 because of a bug in the chromaprint
> >> library\n");
> >> +        goto fail;
> >> +    }
> >> +#endif
> >> +
> >>      if (cpr->silence_threshold != -1) {
> >>  #if CPR_VERSION_INT >= AV_VERSION_INT(0, 7, 0)
> >>          if (!chromaprint_set_option(cpr->ctx, "silence_threshold",
> >> cpr->silence_threshold)) {
> >> --
> >> 2.23.0
> >>
> >
> > The seg fault actually occurs in libavcodec/avfft.c in the
> > av_rdft_calc(RDFTContext *s, FFTSample *data) function, where chromaprint
> > lib parses an unitialized context s=NULL.
> >
> > Is it worth submitting a patch where contexts are checked before
> > dereferencing
> > in avfft.c?
> 

> No, fix chromaprint instead, its very buggy.

ok, I submitted a bug report on their github page.
diff mbox

Patch

diff --git a/libavformat/chromaprint.c b/libavformat/chromaprint.c
index faa92ca0db..3ecce3e08a 100644
--- a/libavformat/chromaprint.c
+++ b/libavformat/chromaprint.c
@@ -70,6 +70,13 @@  static int write_header(AVFormatContext *s)
         return AVERROR(ENOMEM);
     }
 
+#if CPR_VERSION_INT > AV_VERSION_INT(1, 2, 0)
+    if (cpr->algorithm == CHROMAPRINT_ALGORITHM_TEST3) {
+        av_log(s, AV_LOG_ERROR, "Algorithm 2 cannot be used with chromaprint version > 1.2.0 because of a bug in the chromaprint library\n");
+        goto fail;
+    }
+#endif
+
     if (cpr->silence_threshold != -1) {
 #if CPR_VERSION_INT >= AV_VERSION_INT(0, 7, 0)
         if (!chromaprint_set_option(cpr->ctx, "silence_threshold", cpr->silence_threshold)) {