[FFmpeg-devel] avcodec/hevcdec: check that the local context list exists before dereferencing it

Since the decoder is not flagged as init cleanup capable, hevc_decode_free()
is being called manually if the hevc_decode_extradata() call fails at the end
of hevc_decode_init().
In a frame threading scenario, however, if AVCodec->init() returns an error,
ff_frame_thread_free() will be called regardless of the above flag being set
or not, resulting in hevc_decode_free() being called a second time for the
same context.

Solve this by ensuring pointers are not dereferenced if they are NULL, and
setting the decoder as init cleanup capable.

Fixes ticket #9099.

Signed-off-by: James Almer <jamrial@gmail.com>
---
Maybe ff_frame_thread_free() should not call AVCodec->close() for thread contexts
where AVCodec->init() failed and FF_CODEC_CAP_INIT_CLEANUP is not set?

 libavcodec/hevcdec.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

James Almer:
> Since the decoder is not flagged as init cleanup capable, hevc_decode_free()
> is being called manually if the hevc_decode_extradata() call fails at the end
> of hevc_decode_init().
> In a frame threading scenario, however, if AVCodec->init() returns an error,
> ff_frame_thread_free() will be called regardless of the above flag being set
> or not, resulting in hevc_decode_free() being called a second time for the
> same context.
> 
> Solve this by ensuring pointers are not dereferenced if they are NULL, and
> setting the decoder as init cleanup capable.
> 
> Fixes ticket #9099.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Maybe ff_frame_thread_free() should not call AVCodec->close() for thread contexts
> where AVCodec->init() failed and FF_CODEC_CAP_INIT_CLEANUP is not set?
> 

Fixing this has been on my to-do list. (The situation is even worse than
you describe it: It is possible that AVCodec->close is called on an
AVCodecContext whose private_data couldn't be allocated.)

>  libavcodec/hevcdec.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> index 92eb888033..898dac8cbb 100644
> --- a/libavcodec/hevcdec.c
> +++ b/libavcodec/hevcdec.c
> @@ -3417,6 +3417,7 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx)
>      av_freep(&s->sh.offset);
>      av_freep(&s->sh.size);
>  
> +    if (s->HEVClcList && s->sList) {
>      for (i = 1; i < s->threads_number; i++) {
>          HEVCLocalContext *lc = s->HEVClcList[i];
>          if (lc) {
> @@ -3424,9 +3425,8 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx)
>              av_freep(&s->sList[i]);
>          }
>      }
> -    if (s->HEVClc == s->HEVClcList[0])
> -        s->HEVClc = NULL;
> -    av_freep(&s->HEVClcList[0]);
> +    }
> +    av_freep(&s->HEVClc);
>      av_freep(&s->HEVClcList);
>      av_freep(&s->sList);
>  
> @@ -3622,7 +3622,6 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx)
>          if (avctx->extradata_size > 0 && avctx->extradata) {
>              ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size, 1);
>              if (ret < 0) {
> -                hevc_decode_free(avctx);
>                  return ret;
>              }
>          }
> @@ -3673,7 +3672,7 @@ AVCodec ff_hevc_decoder = {
>      .capabilities          = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY |
>                               AV_CODEC_CAP_SLICE_THREADS | AV_CODEC_CAP_FRAME_THREADS,
>      .caps_internal         = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_EXPORTS_CROPPING |
> -                             FF_CODEC_CAP_ALLOCATE_PROGRESS,
> +                             FF_CODEC_CAP_ALLOCATE_PROGRESS | FF_CODEC_CAP_INIT_CLEANUP,
>      .profiles              = NULL_IF_CONFIG_SMALL(ff_hevc_profiles),
>      .hw_configs            = (const AVCodecHWConfigInternal *const []) {
>  #if CONFIG_HEVC_DXVA2_HWACCEL
>

On Wed, Feb 10, 2021 at 6:57 PM Andreas Rheinhardt <
andreas.rheinhardt@gmail.com> wrote:

> James Almer:
> > Since the decoder is not flagged as init cleanup capable,
> hevc_decode_free()
> > is being called manually if the hevc_decode_extradata() call fails at
> the end
> > of hevc_decode_init().
> > In a frame threading scenario, however, if AVCodec->init() returns an
> error,
> > ff_frame_thread_free() will be called regardless of the above flag being
> set
> > or not, resulting in hevc_decode_free() being called a second time for
> the
> > same context.
> >
> > Solve this by ensuring pointers are not dereferenced if they are NULL,
> and
> > setting the decoder as init cleanup capable.
> >
> > Fixes ticket #9099.
> >
> > Signed-off-by: James Almer <jamrial@gmail.com>
> > ---
> > Maybe ff_frame_thread_free() should not call AVCodec->close() for thread
> contexts
> > where AVCodec->init() failed and FF_CODEC_CAP_INIT_CLEANUP is not set?
> >
>
> Fixing this has been on my to-do list. (The situation is even worse than
> you describe it: It is possible that AVCodec->close is called on an
> AVCodecContext whose private_data couldn't be allocated.)
>

So how should proceed? Apply this patch and fix other issues after it?

>
> >  libavcodec/hevcdec.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> > index 92eb888033..898dac8cbb 100644
> > --- a/libavcodec/hevcdec.c
> > +++ b/libavcodec/hevcdec.c
> > @@ -3417,6 +3417,7 @@ static av_cold int hevc_decode_free(AVCodecContext
> *avctx)
> >      av_freep(&s->sh.offset);
> >      av_freep(&s->sh.size);
> >
> > +    if (s->HEVClcList && s->sList) {
> >      for (i = 1; i < s->threads_number; i++) {
> >          HEVCLocalContext *lc = s->HEVClcList[i];
> >          if (lc) {
> > @@ -3424,9 +3425,8 @@ static av_cold int hevc_decode_free(AVCodecContext
> *avctx)
> >              av_freep(&s->sList[i]);
> >          }
> >      }
> > -    if (s->HEVClc == s->HEVClcList[0])
> > -        s->HEVClc = NULL;
> > -    av_freep(&s->HEVClcList[0]);
> > +    }
> > +    av_freep(&s->HEVClc);
> >      av_freep(&s->HEVClcList);
> >      av_freep(&s->sList);
> >
> > @@ -3622,7 +3622,6 @@ static av_cold int hevc_decode_init(AVCodecContext
> *avctx)
> >          if (avctx->extradata_size > 0 && avctx->extradata) {
> >              ret = hevc_decode_extradata(s, avctx->extradata,
> avctx->extradata_size, 1);
> >              if (ret < 0) {
> > -                hevc_decode_free(avctx);
> >                  return ret;
> >              }
> >          }
> > @@ -3673,7 +3672,7 @@ AVCodec ff_hevc_decoder = {
> >      .capabilities          = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY |
> >                               AV_CODEC_CAP_SLICE_THREADS |
> AV_CODEC_CAP_FRAME_THREADS,
> >      .caps_internal         = FF_CODEC_CAP_INIT_THREADSAFE |
> FF_CODEC_CAP_EXPORTS_CROPPING |
> > -                             FF_CODEC_CAP_ALLOCATE_PROGRESS,
> > +                             FF_CODEC_CAP_ALLOCATE_PROGRESS |
> FF_CODEC_CAP_INIT_CLEANUP,
> >      .profiles              = NULL_IF_CONFIG_SMALL(ff_hevc_profiles),
> >      .hw_configs            = (const AVCodecHWConfigInternal *const []) {
> >  #if CONFIG_HEVC_DXVA2_HWACCEL
> >
>
> _______________________________________________
> 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".

hevcdec and pthread_slice has some memory manage issues.
1. hls_slice_data_wpp did not check the return value of ff_alloc_entries
and av_malloc.
2. ff_alloc_entries did not check the return value of pthread_cond_init
and pthread_mutex_init
2. Even hls_slice_data_wpp return some error for memory allocation
failed, decode_nal_unit may ignore the return value at the end, We need
carefully handle the half created arrays/memory.

On 2/10/2021 8:59 PM, Paul B Mahol wrote:
> On Wed, Feb 10, 2021 at 6:57 PM Andreas Rheinhardt <
> andreas.rheinhardt@gmail.com> wrote:
> 
>> James Almer:
>>> Since the decoder is not flagged as init cleanup capable,
>> hevc_decode_free()
>>> is being called manually if the hevc_decode_extradata() call fails at
>> the end
>>> of hevc_decode_init().
>>> In a frame threading scenario, however, if AVCodec->init() returns an
>> error,
>>> ff_frame_thread_free() will be called regardless of the above flag being
>> set
>>> or not, resulting in hevc_decode_free() being called a second time for
>> the
>>> same context.
>>>
>>> Solve this by ensuring pointers are not dereferenced if they are NULL,
>> and
>>> setting the decoder as init cleanup capable.
>>>
>>> Fixes ticket #9099.
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> Maybe ff_frame_thread_free() should not call AVCodec->close() for thread
>> contexts
>>> where AVCodec->init() failed and FF_CODEC_CAP_INIT_CLEANUP is not set?
>>>
>>
>> Fixing this has been on my to-do list. (The situation is even worse than
>> you describe it: It is possible that AVCodec->close is called on an
>> AVCodecContext whose private_data couldn't be allocated.)
>>
> 
> So how should proceed? Apply this patch and fix other issues after it?

Applied this patch. The other unchecked allocs are handled in another 
patch, and the ff_frame_thread_init() issues should be fixed by Andreas' 
patch.