Thanks, this works great. Stand by for patch. On Wed, Feb 6, 2019 at 8:38 AM Hendrik Leppkes <h.lepp...@gmail.com> wrote:
> On Thu, Jan 31, 2019 at 11:29 PM Chris Cunningham > <chcunning...@chromium.org> wrote: > > > > 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? > > 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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
