[FFmpeg-devel,v1] libavcodec/pthread_frame: fix crash that call method ff_frame_thread_init failed because of mem insufficient

From: xuhuishu <xuhuishu.xhs@alibaba-inc.com>

Signed-off-by: xuhuishu <xuhuishu.xhs@alibaba-inc.com>
---
 libavcodec/pthread_frame.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

javashu2012@gmail.com:
> From: xuhuishu <xuhuishu.xhs@alibaba-inc.com>
> 
> Signed-off-by: xuhuishu <xuhuishu.xhs@alibaba-inc.com>
> ---
>  libavcodec/pthread_frame.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index f8a01ad8cd..2babeb4a6a 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -795,6 +795,11 @@ int ff_frame_thread_init(AVCodecContext *avctx)
>          pthread_cond_init(&p->progress_cond, NULL);
>          pthread_cond_init(&p->output_cond, NULL);
>  
> +        if (!copy) {
> +            err = AVERROR(ENOMEM);
> +            goto error;
> +        }
> +
>          p->frame = av_frame_alloc();
>          if (!p->frame) {
>              av_freep(&copy);
> @@ -802,22 +807,18 @@ int ff_frame_thread_init(AVCodecContext *avctx)
>              goto error;
>          }
>  
> -        p->parent = fctx;
> -        p->avctx  = copy;
> -
> -        if (!copy) {
> +        AVCodecInternal *internal = av_malloc(sizeof(AVCodecInternal));
> +        if (!internal) {
> +            av_freep(&copy);
>              err = AVERROR(ENOMEM);
>              goto error;
>          }
>  
> -        *copy = *src;
> +        p->parent = fctx;
> +        p->avctx  = copy;
>  
> -        copy->internal = av_malloc(sizeof(AVCodecInternal));
> -        if (!copy->internal) {
> -            copy->priv_data = NULL;
> -            err = AVERROR(ENOMEM);
> -            goto error;
> -        }
> +        *copy = *src;
> +        copy->internal = internal;
>          *copy->internal = *src->internal;
>          copy->internal->thread_ctx = p;
>          copy->internal->last_pkt_props = &p->avpkt;
> 
How did you test this? Because it does not completely fix the issue:
ff_frame_thread_free() thinks that i+1 AVCodecContexts are to be freed,
but in case of error the last one is not properly initialized. E.g. if
allocating the copy's priv_data fails, ff_frame_thread_free() will
nevertheless attempt to call the codec's close function. And the same
happens when init fails even when the codec does not have the
FF_CODEC_CAP_INIT_CLEANUP set.

- Andreas

Andreas Rheinhardt <andreas.rheinhardt@gmail.com> 于2020年10月16日周五 下午4:46写道：

> javashu2012@gmail.com:
> > From: xuhuishu <xuhuishu.xhs@alibaba-inc.com>
> >
> > Signed-off-by: xuhuishu <xuhuishu.xhs@alibaba-inc.com>
> > ---
> >  libavcodec/pthread_frame.c | 23 ++++++++++++-----------
> >  1 file changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> > index f8a01ad8cd..2babeb4a6a 100644
> > --- a/libavcodec/pthread_frame.c
> > +++ b/libavcodec/pthread_frame.c
> > @@ -795,6 +795,11 @@ int ff_frame_thread_init(AVCodecContext *avctx)
> >          pthread_cond_init(&p->progress_cond, NULL);
> >          pthread_cond_init(&p->output_cond, NULL);
> >
> > +        if (!copy) {
> > +            err = AVERROR(ENOMEM);
> > +            goto error;
> > +        }
> > +
> >          p->frame = av_frame_alloc();
> >          if (!p->frame) {
> >              av_freep(&copy);
> > @@ -802,22 +807,18 @@ int ff_frame_thread_init(AVCodecContext *avctx)
> >              goto error;
> >          }
> >
> > -        p->parent = fctx;
> > -        p->avctx  = copy;
> > -
> > -        if (!copy) {
> > +        AVCodecInternal *internal = av_malloc(sizeof(AVCodecInternal));
> > +        if (!internal) {
> > +            av_freep(&copy);
> >              err = AVERROR(ENOMEM);
> >              goto error;
> >          }
> >
> > -        *copy = *src;
> > +        p->parent = fctx;
> > +        p->avctx  = copy;
> >
> > -        copy->internal = av_malloc(sizeof(AVCodecInternal));
> > -        if (!copy->internal) {
> > -            copy->priv_data = NULL;
> > -            err = AVERROR(ENOMEM);
> > -            goto error;
> > -        }
> > +        *copy = *src;
> > +        copy->internal = internal;
> >          *copy->internal = *src->internal;
> >          copy->internal->thread_ctx = p;
> >          copy->internal->last_pkt_props = &p->avpkt;
> >
> How did you test this? Because it does not completely fix the issue:
> ff_frame_thread_free() thinks that i+1 AVCodecContexts are to be freed,
> but in case of error the last one is not properly initialized. E.g. if
> allocating the copy's priv_data fails, ff_frame_thread_free() will
> nevertheless attempt to call the codec's close function. And the same
> happens when init fails even when the codec does not have the
> FF_CODEC_CAP_INIT_CLEANUP set.
>
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".



so sorry, This is my first ffmpeg patch submission, and I started to plan
to submit it twice, which is very wrong.
I have resubmitted it, I hope you see if there is anything else