diff mbox series

[FFmpeg-devel,5/5] avformat/oggdec: Disable mid stream codec changes

Message ID 20200613112345.13515-5-michael@niedermayer.cc
State Accepted
Headers show
Series [FFmpeg-devel,1/5] avcodec/iff: Fix off by x error
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Michael Niedermayer June 13, 2020, 11:23 a.m. UTC
The code crashes and neither the authors nor anyone else did fix this
We cannot release code which crashes, so if noone fixes it, the only
option left is to disable or revert. Revert is difficult as there are
multiple commits afterwards

Fixes: 22082/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5688619118624768
Fixes: crash from V-codecs/Theora/theora_testsuite_broken/multi2.ogg

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/oggdec.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Lynne June 13, 2020, 12:12 p.m. UTC | #1
Jun 13, 2020, 12:23 by michael@niedermayer.cc:

> The code crashes and neither the authors nor anyone else did fix this
> We cannot release code which crashes, so if noone fixes it, the only
> option left is to disable or revert. Revert is difficult as there are
> multiple commits afterwards
>

You're suggesting a revert when disabling it is so simple?
As if the commits didn't fix an issue users have been begging for for years?
Do you want a permanently dead project where no code changes because it may crash
or break things? You should fork instead, we'd all be better off then.

I even asked you on IRC whether your previous 2 commits fixed the crash, so I know whether
to try fixing it, and you didn't even respond. You didn't even ping me anytime to check up.
And yet you claim I've abandoned it? As if I don't have a million things to work on or worry
about already.
Paul B Mahol June 13, 2020, 1:10 p.m. UTC | #2
On 6/13/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> The code crashes and neither the authors nor anyone else did fix this
> We cannot release code which crashes, so if noone fixes it, the only
> option left is to disable or revert. Revert is difficult as there are
> multiple commits afterwards
>
> Fixes:
> 22082/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5688619118624768
> Fixes: crash from V-codecs/Theora/theora_testsuite_broken/multi2.ogg
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/oggdec.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> index 9eb45499c6..1f3ed8024c 100644
> --- a/libavformat/oggdec.c
> +++ b/libavformat/oggdec.c
> @@ -229,6 +229,15 @@ static int ogg_replace_stream(AVFormatContext *s,
> uint32_t serial, char *magic,
>      /* We only have a single stream anyway, so if there's a new stream with
>       * a different codec just replace it */
>      os = &ogg->streams[0];
> +    if (os->codec != codec) {
> +        /*
> +         * The codec change code from
> 8296443a70f052a6f5c9a867d28b83a5eb7d304d and surounding commits
> +         * crashes with out of array accesses
> +         * testcase is
> https://samples.ffmpeg.org/V-codecs/Theora/theora_testsuite_broken/multi2.ogg
> +         */
> +        return AVERROR_PATCHWELCOME;
> +    }
> +
>      os->serial  = serial;
>      os->codec   = codec;
>      os->serial  = serial;
> --
> 2.17.1
>
> _______________________________________________
> 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".

NAK

It very aggressive and also pointless.
James Almer June 13, 2020, 2:18 p.m. UTC | #3
On 6/13/2020 9:12 AM, Lynne wrote:
> Jun 13, 2020, 12:23 by michael@niedermayer.cc:
> 
>> The code crashes and neither the authors nor anyone else did fix this
>> We cannot release code which crashes, so if noone fixes it, the only
>> option left is to disable or revert. Revert is difficult as there are
>> multiple commits afterwards
>>
> 
> You're suggesting a revert when disabling it is so simple?
> As if the commits didn't fix an issue users have been begging for for years?
> Do you want a permanently dead project where no code changes because it may crash
> or break things? You should fork instead, we'd all be better off then.

It would be very helpful too if you could stop being so goddamn
aggressive with every subject.

> 
> I even asked you on IRC whether your previous 2 commits fixed the crash, so I know whether
> to try fixing it, and you didn't even respond. You didn't even ping me anytime to check up.
> And yet you claim I've abandoned it? As if I don't have a million things to work on or worry
> about already.

This patch was a good opportunity to ping him about your question that
went unanswered on IRC, while also NAKing the patch itself, instead of
the above.
Michael Niedermayer June 13, 2020, 10:52 p.m. UTC | #4
On Sat, Jun 13, 2020 at 02:12:55PM +0200, Lynne wrote:
> Jun 13, 2020, 12:23 by michael@niedermayer.cc:
> 
> > The code crashes and neither the authors nor anyone else did fix this
> > We cannot release code which crashes, so if noone fixes it, the only
> > option left is to disable or revert. Revert is difficult as there are
> > multiple commits afterwards
> >
> 
> You're suggesting a revert when disabling it is so simple?
> As if the commits didn't fix an issue users have been begging for for years?
> Do you want a permanently dead project where no code changes because it may crash
> or break things? You should fork instead, we'd all be better off then.
> 
> I even asked you on IRC whether your previous 2 commits fixed the crash, so I know whether
> to try fixing it, and you didn't even respond. You didn't even ping me anytime to check up.
> And yet you claim I've abandoned it? As if I don't have a million things to work on or worry
> about already.

I missed your questions on IRC, in fact ive been a little sick in the last
few days, nothing serious, just some migraine and mild fever after a dentist.
But that probably kept me from looking at IRC for too long at the wrong time

sorry

[...]
Jean-Baptiste Kempf June 14, 2020, 3:33 p.m. UTC | #5
On Sat, Jun 13, 2020, at 16:18, James Almer wrote:
> On 6/13/2020 9:12 AM, Lynne wrote:
> > Jun 13, 2020, 12:23 by michael@niedermayer.cc:
> > I even asked you on IRC whether your previous 2 commits fixed the crash, so I know whether
> > to try fixing it, and you didn't even respond. You didn't even ping me anytime to check up.
> > And yet you claim I've abandoned it? As if I don't have a million things to work on or worry
> > about already.
> 
> This patch was a good opportunity to ping him about your question that
> went unanswered on IRC, while also NAKing the patch itself, instead of
> the above.

Can we have a fix without the drama, people?
Come on, there is nothing unfixable here.
diff mbox series

Patch

diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 9eb45499c6..1f3ed8024c 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -229,6 +229,15 @@  static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, char *magic,
     /* We only have a single stream anyway, so if there's a new stream with
      * a different codec just replace it */
     os = &ogg->streams[0];
+    if (os->codec != codec) {
+        /*
+         * The codec change code from 8296443a70f052a6f5c9a867d28b83a5eb7d304d and surounding commits
+         * crashes with out of array accesses
+         * testcase is https://samples.ffmpeg.org/V-codecs/Theora/theora_testsuite_broken/multi2.ogg
+         */
+        return AVERROR_PATCHWELCOME;
+    }
+
     os->serial  = serial;
     os->codec   = codec;
     os->serial  = serial;