diff mbox series

[FFmpeg-devel,2/4] libavutil/hwcontext_qsv: fix a bug when malloc handle_pairs_internal

Message ID 20211104054403.1251208-2-wenbin.chen@intel.com
State New
Headers show
Series [FFmpeg-devel,1/4] libavutil/hwcontext_d3d11va: Add nb_surfaces to AVD3D11VAFramesContext
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Wenbin Chen Nov. 4, 2021, 5:44 a.m. UTC
This commandline cause core dumped:
ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 \
-hwaccel_output_format vaapi -i input.264 \
-vf "hwmap=derive_device=qsv,format=qsv" \
-c:v h264_qsv output.264

reason: We use nb_surfaces to assign surface to handle_pairs_internal
but handle_pairs_internal is alloced with the size of init_pool_size.
This lead to access to illegal address.

Now change it to use nb_surfaces to allocate handle_pairs_internal and the
core dumped error is unseen. Also change D3D11VA to use nb_surfaces
to align to VAAPI and DXVA2.

Signed-off-by: Wenbin Chen <wenbin.chen@intel.com>
---
 libavutil/hwcontext_qsv.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

Wenbin Chen Nov. 10, 2021, 3:03 a.m. UTC | #1
> This commandline cause core dumped:
> ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 \
> -hwaccel_output_format vaapi -i input.264 \
> -vf "hwmap=derive_device=qsv,format=qsv" \
> -c:v h264_qsv output.264
> 
> reason: We use nb_surfaces to assign surface to handle_pairs_internal
> but handle_pairs_internal is alloced with the size of init_pool_size.
> This lead to access to illegal address.
> 
> Now change it to use nb_surfaces to allocate handle_pairs_internal and the
> core dumped error is unseen. Also change D3D11VA to use nb_surfaces
> to align to VAAPI and DXVA2.
> 
> Signed-off-by: Wenbin Chen <wenbin.chen@intel.com>
> ---
>  libavutil/hwcontext_qsv.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index c18747f7eb..5a285fd25b 100644
> --- a/libavutil/hwcontext_qsv.c
> +++ b/libavutil/hwcontext_qsv.c
> @@ -1123,8 +1123,7 @@ static int
> qsv_frames_derive_to(AVHWFramesContext *dst_ctx,
>      case AV_HWDEVICE_TYPE_VAAPI:
>          {
>              AVVAAPIFramesContext *src_hwctx = src_ctx->hwctx;
> -            s->handle_pairs_internal = av_calloc(src_ctx->initial_pool_size,
> -                                                 sizeof(*s->handle_pairs_internal));
> +            s->handle_pairs_internal = av_calloc(src_hwctx->nb_surfaces,
> sizeof(*s->handle_pairs_internal));
>              if (!s->handle_pairs_internal)
>                  return AVERROR(ENOMEM);
>              s->surfaces_internal = av_calloc(src_hwctx->nb_surfaces,
> @@ -1146,15 +1145,15 @@ static int
> qsv_frames_derive_to(AVHWFramesContext *dst_ctx,
>      case AV_HWDEVICE_TYPE_D3D11VA:
>          {
>              AVD3D11VAFramesContext *src_hwctx = src_ctx->hwctx;
> -            s->handle_pairs_internal = av_calloc(src_ctx->initial_pool_size,
> +            s->handle_pairs_internal = av_calloc(src_ctx->nb_surfaces,
>                                                   sizeof(*s->handle_pairs_internal));
>              if (!s->handle_pairs_internal)
>                  return AVERROR(ENOMEM);
> -            s->surfaces_internal = av_calloc(src_ctx->initial_pool_size,
> +            s->surfaces_internal = av_calloc(src_ctx->nb_surfaces,
>                                               sizeof(*s->surfaces_internal));
>              if (!s->surfaces_internal)
>                  return AVERROR(ENOMEM);
> -            for (i = 0; i < src_ctx->initial_pool_size; i++) {
> +            for (i = 0; i < src_ctx->nb_surfaces; i++) {
>                  qsv_init_surface(dst_ctx, &s->surfaces_internal[i]);
>                  s->handle_pairs_internal[i].first = (mfxMemId)src_hwctx-
> >texture_infos[i].texture;
>                  if (src_hwctx->BindFlags & D3D11_BIND_RENDER_TARGET) {
> @@ -1164,7 +1163,7 @@ static int
> qsv_frames_derive_to(AVHWFramesContext *dst_ctx,
>                  }
>                  s->surfaces_internal[i].Data.MemId = (mfxMemId)&s-
> >handle_pairs_internal[i];
>              }
> -            dst_hwctx->nb_surfaces = src_ctx->initial_pool_size;
> +            dst_hwctx->nb_surfaces = src_ctx->nb_surfaces;
>              if (src_hwctx->BindFlags & D3D11_BIND_RENDER_TARGET) {
>                  dst_hwctx->frame_type |=
> MFX_MEMTYPE_VIDEO_MEMORY_PROCESSOR_TARGET;
>              } else {
> @@ -1177,7 +1176,7 @@ static int
> qsv_frames_derive_to(AVHWFramesContext *dst_ctx,
>      case AV_HWDEVICE_TYPE_DXVA2:
>          {
>              AVDXVA2FramesContext *src_hwctx = src_ctx->hwctx;
> -            s->handle_pairs_internal = av_calloc(src_ctx->initial_pool_size,
> +            s->handle_pairs_internal = av_calloc(src_ctx->nb_surfaces,
>                                                   sizeof(*s->handle_pairs_internal));
>              if (!s->handle_pairs_internal)
>                  return AVERROR(ENOMEM);
> --
> 2.25.1

ping
Soft Works Nov. 10, 2021, 8:38 a.m. UTC | #2
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Chen, Wenbin
> Sent: Wednesday, November 10, 2021 4:03 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 2/4] libavutil/hwcontext_qsv: fix
> a bug when malloc handle_pairs_internal
> 
> > This commandline cause core dumped:
> > ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 \
> > -hwaccel_output_format vaapi -i input.264 \
> > -vf "hwmap=derive_device=qsv,format=qsv" \
> > -c:v h264_qsv output.264
> >
> > reason: We use nb_surfaces to assign surface to
> handle_pairs_internal
> > but handle_pairs_internal is alloced with the size of
> init_pool_size.
> > This lead to access to illegal address.
> >
> > Now change it to use nb_surfaces to allocate handle_pairs_internal
> and the

I'm not sure about whether this is right.

When we look at the top of the qsv_frames_derive_to function that you 
are changing, there is this:


    if (src_ctx->initial_pool_size == 0) {
        av_log(dst_ctx, AV_LOG_ERROR, "Only fixed-size pools can be "
            "mapped to QSV frames.\n");
        return AVERROR(EINVAL);
    }

It's because QSV doesn't support dynamic pool sizes.

When we look at the vaapi_pool_alloc function in hwcontext_vaapi.c, we
can see that:

  when  initial_pool_size is > 0, the pool cannot grow beyond this value,
  so nb_surfaces cannot be > initial_pool_size

So I'm wondering what could have caused the segfault? Which values did
you have there for nb_surfaces and initial_pool_size?


> > core dumped error is unseen. Also change D3D11VA to use nb_surfaces
> > to align to VAAPI and DXVA2.

Those changes are unrelated to fixing the issue with VAAPI.
(besides that I don't think these are needed at all)

Kind regards,
softworkz
Wenbin Chen Nov. 11, 2021, 7:35 a.m. UTC | #3
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Chen, Wenbin
> > Sent: Wednesday, November 10, 2021 4:03 AM
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH 2/4] libavutil/hwcontext_qsv: fix
> > a bug when malloc handle_pairs_internal
> >
> > > This commandline cause core dumped:
> > > ffmpeg -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 \
> > > -hwaccel_output_format vaapi -i input.264 \
> > > -vf "hwmap=derive_device=qsv,format=qsv" \
> > > -c:v h264_qsv output.264
> > >
> > > reason: We use nb_surfaces to assign surface to
> > handle_pairs_internal
> > > but handle_pairs_internal is alloced with the size of
> > init_pool_size.
> > > This lead to access to illegal address.
> > >
> > > Now change it to use nb_surfaces to allocate handle_pairs_internal
> > and the
> 
> I'm not sure about whether this is right.
> 
> When we look at the top of the qsv_frames_derive_to function that you
> are changing, there is this:
> 
> 
>     if (src_ctx->initial_pool_size == 0) {
>         av_log(dst_ctx, AV_LOG_ERROR, "Only fixed-size pools can be "
>             "mapped to QSV frames.\n");
>         return AVERROR(EINVAL);
>     }
> 
> It's because QSV doesn't support dynamic pool sizes.
> 
> When we look at the vaapi_pool_alloc function in hwcontext_vaapi.c, we
> can see that:
> 
>   when  initial_pool_size is > 0, the pool cannot grow beyond this value,
>   so nb_surfaces cannot be > initial_pool_size
> 
> So I'm wondering what could have caused the segfault? Which values did
> you have there for nb_surfaces and initial_pool_size?
> 
> 
> > > core dumped error is unseen. Also change D3D11VA to use nb_surfaces
> > > to align to VAAPI and DXVA2.
> 
> Those changes are unrelated to fixing the issue with VAAPI.
> (besides that I don't think these are needed at all)
> 
> Kind regards,
> softworkz

You are right. The real cause is that vaapi_decode_make_config() is called twice.
The init_pool_size is changed on the second call. I will resubmit patch to fix this

Thanks
Wenbin
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
index c18747f7eb..5a285fd25b 100644
--- a/libavutil/hwcontext_qsv.c
+++ b/libavutil/hwcontext_qsv.c
@@ -1123,8 +1123,7 @@  static int qsv_frames_derive_to(AVHWFramesContext *dst_ctx,
     case AV_HWDEVICE_TYPE_VAAPI:
         {
             AVVAAPIFramesContext *src_hwctx = src_ctx->hwctx;
-            s->handle_pairs_internal = av_calloc(src_ctx->initial_pool_size,
-                                                 sizeof(*s->handle_pairs_internal));
+            s->handle_pairs_internal = av_calloc(src_hwctx->nb_surfaces, sizeof(*s->handle_pairs_internal));
             if (!s->handle_pairs_internal)
                 return AVERROR(ENOMEM);
             s->surfaces_internal = av_calloc(src_hwctx->nb_surfaces,
@@ -1146,15 +1145,15 @@  static int qsv_frames_derive_to(AVHWFramesContext *dst_ctx,
     case AV_HWDEVICE_TYPE_D3D11VA:
         {
             AVD3D11VAFramesContext *src_hwctx = src_ctx->hwctx;
-            s->handle_pairs_internal = av_calloc(src_ctx->initial_pool_size,
+            s->handle_pairs_internal = av_calloc(src_ctx->nb_surfaces,
                                                  sizeof(*s->handle_pairs_internal));
             if (!s->handle_pairs_internal)
                 return AVERROR(ENOMEM);
-            s->surfaces_internal = av_calloc(src_ctx->initial_pool_size,
+            s->surfaces_internal = av_calloc(src_ctx->nb_surfaces,
                                              sizeof(*s->surfaces_internal));
             if (!s->surfaces_internal)
                 return AVERROR(ENOMEM);
-            for (i = 0; i < src_ctx->initial_pool_size; i++) {
+            for (i = 0; i < src_ctx->nb_surfaces; i++) {
                 qsv_init_surface(dst_ctx, &s->surfaces_internal[i]);
                 s->handle_pairs_internal[i].first = (mfxMemId)src_hwctx->texture_infos[i].texture;
                 if (src_hwctx->BindFlags & D3D11_BIND_RENDER_TARGET) {
@@ -1164,7 +1163,7 @@  static int qsv_frames_derive_to(AVHWFramesContext *dst_ctx,
                 }
                 s->surfaces_internal[i].Data.MemId = (mfxMemId)&s->handle_pairs_internal[i];
             }
-            dst_hwctx->nb_surfaces = src_ctx->initial_pool_size;
+            dst_hwctx->nb_surfaces = src_ctx->nb_surfaces;
             if (src_hwctx->BindFlags & D3D11_BIND_RENDER_TARGET) {
                 dst_hwctx->frame_type |= MFX_MEMTYPE_VIDEO_MEMORY_PROCESSOR_TARGET;
             } else {
@@ -1177,7 +1176,7 @@  static int qsv_frames_derive_to(AVHWFramesContext *dst_ctx,
     case AV_HWDEVICE_TYPE_DXVA2:
         {
             AVDXVA2FramesContext *src_hwctx = src_ctx->hwctx;
-            s->handle_pairs_internal = av_calloc(src_ctx->initial_pool_size,
+            s->handle_pairs_internal = av_calloc(src_ctx->nb_surfaces,
                                                  sizeof(*s->handle_pairs_internal));
             if (!s->handle_pairs_internal)
                 return AVERROR(ENOMEM);