On Wed, Jan 30, 2019 at 5:43 PM James Almer <jamr...@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?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
