[FFmpeg-devel] [PATCH] avformat/oggdec: only parse headers before data

[Chris Cunningham]

On Wed, Jun 19, 2019 at 7:11 PM Chris Cunningham <chcunningham at chromium.org>
wrote:

> On Wed, Jun 19, 2019 at 11:25 AM Michael Niedermayer
> <michael at niedermayer.cc> wrote:
>
>> breaks:
>> ./ffmpeg -i bgc.sub.dub.ogm -vframes 3 -y test.webm
>> sample: http://samples.mplayerhq.hu/ogg/bgc.sub.dub.ogm
>>
>> [...]
>>
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>
> Thanks Michael.
>
> That file is example of the "invalid" sort we were discussing previously.
> As I understand it, the spec requires that data packets not be interleaved
> with header packets - the headers for all streams should conclude before
> data packets begin. In this file we see a few headers followed by a data
> packet (stream 0), followed by more headers for streams 1 - 3.
>
> We knew this change would break such files. Can we live it? James, any
> workaround? If not I'm still open to
> setting st->internal->need_context_update as discussed in the earlier patch
> (https://patchwork.ffmpeg.org/patch/11983/)
>
> Chris
>

Quick refresher on my fuzzer input

avformat_find_stream_info finds 3 streams
[0] AV_CODEC_ID_OPUS
[1] AV_CODEC_ID_TEXT
[2] AV_CODEC_ID_NONE

A bit later we're reading frames out and stream 2 encounters a header that
changes the codec from NONE -> AV_CODEC_ID_GSM_MS. ogm_header() assigns
this to codecpar->codec_id, but st->internal->avctx->codec_id is never
updated (remains NONE). This mismatch ultimately triggers an assert0 in
gsm_parse (expecting only gsm codecs).

Things tried so far:
- keep codec internal in sync (need_context_update = 1)
https://patchwork.ffmpeg.org/patch/11983/
- don't allow codec id = none in ogm parse
https://patchwork.ffmpeg.org/patch/13529/
- and disallow headers after data (this thread)

I think this last one misses the mark slightly. It happens that the a codec
change corresponds with a header that comes after data packets start, but I
think it could just as easily have occurred without that interleaved data
packet. The root question is whether there is some way to know "this header
is garbage" ... but a header that assigns a reasonable gsm codec
(particularly when the codec was not yet known) seems pretty reasonable.