[FFmpeg-devel] avformat/utils: fix crashes in has_decode_delay_been_guessed

These paths can be taken when the actual underlying codec is not h264,
but the user forces, for example via ffmpeg command line, a specific
input decoder that happens to be a h264 decoder.

In that case, the codecpar codec_id is set to h264, but the internal
avctx is the one of, for example, an mpeg2 decoder, thus crashing in
this function.

Checking for the codec actually being h264 is not strictly neccesary to
fix the crash, but a precaution to catch potential other unexpected
codepaths.

Fixes #5985
---
 libavformat/utils.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

On Fri,  2 Dec 2016 13:51:24 +0100
Timo Rothenpieler <timo@rothenpieler.org> wrote:

> These paths can be taken when the actual underlying codec is not h264,
> but the user forces, for example via ffmpeg command line, a specific
> input decoder that happens to be a h264 decoder.
> 
> In that case, the codecpar codec_id is set to h264, but the internal
> avctx is the one of, for example, an mpeg2 decoder, thus crashing in
> this function.
> 
> Checking for the codec actually being h264 is not strictly neccesary to
> fix the crash, but a precaution to catch potential other unexpected
> codepaths.
> 
> Fixes #5985
> ---
>  libavformat/utils.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 345bbfe5fe..8e23c0c6ec 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -966,11 +966,14 @@ static int is_intra_only(enum AVCodecID id)
>  
>  static int has_decode_delay_been_guessed(AVStream *st)
>  {
> -    if (st->codecpar->codec_id != AV_CODEC_ID_H264) return 1;
> +    if (st->codecpar->codec_id != AV_CODEC_ID_H264 ||
> +       st->internal->avctx->codec_id != AV_CODEC_ID_H264)
> +        return 1;
>      if (!st->info) // if we have left find_stream_info then nb_decoded_frames won't increase anymore for stream copy
>          return 1;
>  #if CONFIG_H264_DECODER
> -    if (st->internal->avctx->has_b_frames &&
> +    if (st->internal->avctx->codec && !strcmp(st->internal->avctx->codec->name, "h264") &&
> +       st->internal->avctx->has_b_frames &&
>         avpriv_h264_has_num_reorder_frames(st->internal->avctx) == st->internal->avctx->has_b_frames)
>          return 1;
>  #endif

That looks ok, but: can someone explain to me why Libav does apparently
not need this shitshow?

On Fri, Dec 02, 2016 at 01:51:24PM +0100, Timo Rothenpieler wrote:
> These paths can be taken when the actual underlying codec is not h264,
> but the user forces, for example via ffmpeg command line, a specific
> input decoder that happens to be a h264 decoder.
> 

> In that case, the codecpar codec_id is set to h264, but the internal
> avctx is the one of, for example, an mpeg2 decoder, thus crashing in
> this function.

Is it just me or is this completely inconsistent?
the codec id told to the user is h264 while we internally use a
mpeg2 decoder to analyze it ?

If its h264 (as forced by the user) we should use a h264 decoder
to internally analyze it

[...]

> Is it just me or is this completely inconsistent?
> the codec id told to the user is h264 while we internally use a
> mpeg2 decoder to analyze it ?
> 
> If its h264 (as forced by the user) we should use a h264 decoder
> to internally analyze it
> 

Yes, something is very wrong here.
I also wasn't able to reproduce this with any self made sample. Only the
one from Ticket 5985 makes it crash for me. In two separate ways even.
In one case, the avctx->codec is NULL, because there are unknown codecs
in that sample, and in other cases the codecs mismatch.

I don't have time to take an in depth look at what is going on there, so
for now I decided to harden it against crashes, which is probably a good
idea in any case.

If this patch gets merged, it should also be backported to at least 3.2

On Fri, Dec 02, 2016 at 05:05:29PM +0100, Timo Rothenpieler wrote:
> > Is it just me or is this completely inconsistent?
> > the codec id told to the user is h264 while we internally use a
> > mpeg2 decoder to analyze it ?
> > 
> > If its h264 (as forced by the user) we should use a h264 decoder
> > to internally analyze it
> > 
> 
> Yes, something is very wrong here.
> I also wasn't able to reproduce this with any self made sample. Only the
> one from Ticket 5985 makes it crash for me. In two separate ways even.
> In one case, the avctx->codec is NULL, because there are unknown codecs
> in that sample, and in other cases the codecs mismatch.
> 
> I don't have time to take an in depth look at what is going on there, so
> for now I decided to harden it against crashes, which is probably a good
> idea in any case.

I dont think its a good idea to leave the inconsistency


[...]

ping

If nobody has a better idea how to resolve the crash, I'm going to push
this today.

On Sat, Dec 03, 2016 at 06:00:12PM +0100, Timo Rothenpieler wrote:
> ping
> 
> If nobody has a better idea how to resolve the crash, I'm going to push
> this today.

i dont think this patch really fixes it

with this patch and the same sample:
make -j12 && ./ffmpeg -vcodec h264_cuvid -i full20.ts

...

Assertion avctx->codec_id == s->parser->codec_ids[0] || avctx->codec_id == s->parser->codec_ids[1] || avctx->codec_id == s->parser->codec_ids[2] || avctx->codec_id == s->parser->codec_ids[3] || avctx->codec_id == s->parser->codec_ids[4] failed at libavcodec/parser.c:151
Aborted

[...]