[FFmpeg-devel] [PATCH] avcodec/gsm_parser: return -1 on parse error

[Chris Cunningham]

Thanks, this works great. Stand by for patch.

On Wed, Feb 6, 2019 at 8:38 AM Hendrik Leppkes <h.leppkes at gmail.com> wrote:

> On Thu, Jan 31, 2019 at 11:29 PM Chris Cunningham
> <chcunningham at chromium.org> wrote:
> >
> > On Wed, Jan 30, 2019 at 5:43 PM James Almer <jamrial at gmail.com> wrote:
> >
> > > On 1/30/2019 10:20 PM, Chris Cunningham wrote:
> > > >>
> > > >>>> And this definitely means there's a bug elsewhere. This parser
> should
> > > >>>> have not been used for codecs ids other than GSM and GSM_MS.
> That's
> > > >>>> precisely what this assert() is making sure of.
> > > >>>>
> > > >>>
> > > >>> I'll take a closer look at how this parser was selected.
> > > >>
> > > >
> > > > Ok, here are full details of how this happens:
> > > >
> > > >    1. AV_CODEC_ID_GSM_MS is assigned to on st->codecpar->codec_id
> > > >    in ogm_header() (oggparseogm.c:75)
> > > >    2. The (deprecated) st->codec->codec_id is not assigned at that
> time
> > > and
> > > >    remains 0 (UNKNOWN)
> > > >    3. AV_CODEC_ID_GSM_MS from st->codecpar is passed to
> av_parser_init,
> > > >    selecting the gsm parser (in read_frame_internal())
> > > >    4. The fuzzer next (only) packet has a size of 0, so the first
> call to
> > > >    parse_packet() (still in read_frame_internal()) does nothing
> > > >    5. After this call we assign several members of
> st->internal->avctx to
> > > >    st->codecpar. This leaves codecpar->codec_id = UNKNOWN.
> > > Interestingly, we
> > > >    do not reset the st->parser at this point (maybe we should?)
> > >
> > > Where does this happen? A call to avcodec_parameters_from_context()
> > > should also copy codec_id.
> > >
> >
> > Ignore #5 here - I'm not seeing that today - was likely confused.
> >
> >
> > >
> > > >    6. Next iteration of the read_frame_internal() loop we get EOF
> from
> > > >    ff_read_packet. This triggers the "flush the parsers" call to
> > > >    parse_packet() which finally reaches gsm_parse() to trigger the
> abort
> > > >    (avctx->codec_id is still 0).
> > > >
> > > > Questions (guessing at the fix):
> > > >
> > > >    - At what point should st->codec->codec_id be synchronized with
> > > >    st->codecpar->codec_id? It never is in this test.
> > >
> > > In avformat_find_stream_info(), i think. In any case, st->codec should
> > > have no effect on parsers. What is used in parse_packet() however is
> > > st->internal->avctx, so that context is the one that evidently contains
> > > codec_id == UNKNOWN when it should be in sync with codecpar.
> > >
> >
> > We do call avformat_find_stream_info, and avcodec_parameters_from_context
> > is called during that process. Everything seems OK when
> > avformat_find_stream_info is done. The codecpar->codec_id is in sync with
> > internal->avctx->codec_id for all streams.
> >
> > But then we start calling av_read_frame as part of normal demuxing.
> > avcodec_parameters_from_context() does not appear to be called during
> this
> > process. Eventually we get this stack:
> >
> > ogm_header
> > ogg_packet
> > ogg_read_packet
> > ff_read_packet
> > read_frame_internal
> > av_read_frame
> >
> > Inside ogm_header, st->codecpar->codec_id is assigned AV_CODEC_ID_GSM_MS
> > (previous value was codec NONE). But *st->internal->avctx->codec_id is
> > never updated. It remains NONE for the rest of this test. *
> >
> > When this unwinds back to read_frame_internal, st->parser is assigned
> using
> > this new codec (GSM_MS)
> >     st->parser = av_parser_init(st->codecpar->codec_id);
> >
> > Later on, still inside read_frame_internal's loop, we get EOF and call,
> > parse_packet (/* flush the parsers */)
> > parse_packet() calls av_parser_parse2(), passing st->internal->avctx
> > (codec_id still NONE, while codecpar still says GSM_MS). This ultimately
> > gets passed to gsm_parse, which triggers the assert0.
> >
> > The underlined bit above seems like the root cause. Where should we be
> > updating st->internal->avctx->codec_id?
>
> We have a flag to set that causes avformat to fix its internal state
> if a demuxer changes it after the initial opening.
> st->internal->need_context_update = 1
>
> Try setting that at the position where the demuxer changes codecpar
> (ie. in ogm_header?), and see if that resolves it.
>
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>