[FFmpeg-devel] [PATCH v2] avcodec/decode: guard against NULL hw_frames_ctx

[Rogozhkin, Dmitry V]

On Tue, 2023-11-14 at 11:14 -0800, Dmitry Rogozhkin wrote:
> Guard against segfault running VLC decoding under msys2 [1]:
> 
> Thread 33 received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 37728.0xadd0]
> ff_hwaccel_frame_priv_alloc (avctx=0x6447b00,
> hwaccel_picture_private=0x65dfd00)
>     at libavcodec/decode.c:1848
> 1848        frames_ctx = (AVHWFramesContext *)avctx->hw_frames_ctx-
> >data;
> (gdb) bt
>     at libavcodec/decode.c:1848
>     at libavcodec/h264_slice.c:208
>     first_slice=1) at libavcodec/h264_slice.c:1599
>     at libavcodec/h264_slice.c:2130
>     at libavcodec/h264dec.c:652
>     got_frame=0x646e4b0, avpkt=0x64522c0) at
> libavcodec/h264dec.c:1048
> 
> (gdb) p avctx
> $1 = (AVCodecContext *) 0x6447b00
> (gdb) p avctx->hw_frames_ctx
> $2 = (AVBufferRef *) 0x0
> 
> v2: check for free_frame_priv (Hendrik)
> 
> See[1]: https://github.com/msys2/MINGW-packages/pull/19050

Posting below some comments done in this PR for this patch.

> Fixes: be07145109 ("avcodec: add AVHWAccel.free_frame_priv callback")
> CC: Lynne <dev at lynne.ee>
> CC: Christoph Reiter <reiter.christoph at gmail.com>
> Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin at intel.com>
> ---
>  libavcodec/decode.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index ad39021..58f887d 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -1838,17 +1838,29 @@ int ff_copy_palette(void *dst, const AVPacket
> *src, void *logctx)
>  int ff_hwaccel_frame_priv_alloc(AVCodecContext *avctx, void
> **hwaccel_picture_private)
>  {
>      const FFHWAccel *hwaccel = ffhwaccel(avctx->hwaccel);
> -    AVHWFramesContext *frames_ctx;
>  
>      if (!hwaccel || !hwaccel->frame_priv_data_size)
>          return 0;
>  
>      av_assert0(!*hwaccel_picture_private);
>  
> -    frames_ctx = (AVHWFramesContext *)avctx->hw_frames_ctx->data;
> -    *hwaccel_picture_private = ff_refstruct_alloc_ext(hwaccel-
> >frame_priv_data_size, 0,
> -                                                      frames_ctx-
> >device_ctx,
> -                                                      hwaccel-
> >free_frame_priv);
> +    if (hwaccel->free_frame_priv) {
> +        AVHWFramesContext *frames_ctx;
> +
> +        if (!avctx->hw_frames_ctx)
> +            return AVERROR(ENOMEM);

Similar other places log and return EINVAL:
https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/decode.c#L1121-L1123
ENOMEM seems like the wrong status here since there is no allocation
happening.

> +
> +        frames_ctx = (AVHWFramesContext *) avctx->hw_frames_ctx-
> >data;
> +        if (!frames_ctx)
> +            return AVERROR(ENOMEM);

Can hw_frames_ctx->data really be null here? i.e. is this check needed?
If yes, then ENOMEM looks wrong here too.

> +
> +        *hwaccel_picture_private = ff_refstruct_alloc_ext(hwaccel-
> >frame_priv_data_size, 0,
> +                                                          frames_ctx
> ->device_ctx,
> +                                                          hwaccel-
> >free_frame_priv);
> +    } else {
> +        *hwaccel_picture_private = ff_refstruct_allocz(hwaccel-
> >frame_priv_data_size);
> +    }
> +
>      if (!*hwaccel_picture_private)
>          return AVERROR(ENOMEM);
>