diff mbox

[FFmpeg-devel] avutil/hwcontext_dxva2: Don't improperly free IDirect3DSurface9 objects

Message ID f69648c4-3601-a5c0-a1e5-1361979f5ea7@aracnet.com
State Accepted
Headers show

Commit Message

Aaron Levinson May 16, 2017, 12:04 p.m. UTC
Add dxva2_pool_release_dummy() and use it in call to
av_buffer_create() in dxva2_pool_alloc().

Prior to this change, av_buffer_create() was called with NULL for the
third argument, which indicates that av_buffer_default_free() should
be used to free the buffer's data.  Eventually, it gets to
buffer_pool_free() and calls buf->free() on a surface object (which is
av_buffer_default_free()).

This can result in a crash when the debug version of the C-runtime is
used on Windows.  While it doesn't appear to result in a crash when
the release version of the C-runtime is used on Windows, it likely
results in memory corruption, since av_free() is being called on
memory that was allocated using
IDirectXVideoAccelerationService::CreateSurface().

Signed-off-by: Aaron Levinson <alevinsn@aracnet.com>
---
 libavutil/hwcontext_dxva2.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

wm4 May 16, 2017, 12:23 p.m. UTC | #1
On Tue, 16 May 2017 05:04:36 -0700
Aaron Levinson <alevinsn@aracnet.com> wrote:

> Add dxva2_pool_release_dummy() and use it in call to
> av_buffer_create() in dxva2_pool_alloc().
> 
> Prior to this change, av_buffer_create() was called with NULL for the
> third argument, which indicates that av_buffer_default_free() should
> be used to free the buffer's data.  Eventually, it gets to
> buffer_pool_free() and calls buf->free() on a surface object (which is
> av_buffer_default_free()).
> 
> This can result in a crash when the debug version of the C-runtime is
> used on Windows.  While it doesn't appear to result in a crash when
> the release version of the C-runtime is used on Windows, it likely
> results in memory corruption, since av_free() is being called on
> memory that was allocated using
> IDirectXVideoAccelerationService::CreateSurface().
> 
> Signed-off-by: Aaron Levinson <alevinsn@aracnet.com>
> ---
>  libavutil/hwcontext_dxva2.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/hwcontext_dxva2.c b/libavutil/hwcontext_dxva2.c
> index 4ed0d56..6c41788 100644
> --- a/libavutil/hwcontext_dxva2.c
> +++ b/libavutil/hwcontext_dxva2.c
> @@ -121,6 +121,13 @@ static void dxva2_frames_uninit(AVHWFramesContext *ctx)
>      }
>  }
>  
> +static void dxva2_pool_release_dummy(void *opaque, uint8_t *data)
> +{
> +    // important not to free anything here--data is a surface object
> +    // associated with the call to CreateSurface(), and these surfaces are
> +    // released in dxva2_frames_uninit()
> +}
> +
>  static AVBufferRef *dxva2_pool_alloc(void *opaque, int size)
>  {
>      AVHWFramesContext      *ctx = (AVHWFramesContext*)opaque;
> @@ -130,7 +137,7 @@ static AVBufferRef *dxva2_pool_alloc(void *opaque, int size)
>      if (s->nb_surfaces_used < hwctx->nb_surfaces) {
>          s->nb_surfaces_used++;
>          return av_buffer_create((uint8_t*)s->surfaces_internal[s->nb_surfaces_used - 1],
> -                                sizeof(*hwctx->surfaces), NULL, 0, 0);
> +                                sizeof(*hwctx->surfaces), dxva2_pool_release_dummy, 0, 0);
>      }
>  
>      return NULL;

LGTM
Steven Liu May 16, 2017, 12:45 p.m. UTC | #2
2017-05-16 20:23 GMT+08:00 wm4 <nfxjfg@googlemail.com>:

> On Tue, 16 May 2017 05:04:36 -0700
> Aaron Levinson <alevinsn@aracnet.com> wrote:
>
> > Add dxva2_pool_release_dummy() and use it in call to
> > av_buffer_create() in dxva2_pool_alloc().
> >
> > Prior to this change, av_buffer_create() was called with NULL for the
> > third argument, which indicates that av_buffer_default_free() should
> > be used to free the buffer's data.  Eventually, it gets to
> > buffer_pool_free() and calls buf->free() on a surface object (which is
> > av_buffer_default_free()).
> >
> > This can result in a crash when the debug version of the C-runtime is
> > used on Windows.  While it doesn't appear to result in a crash when
> > the release version of the C-runtime is used on Windows, it likely
> > results in memory corruption, since av_free() is being called on
> > memory that was allocated using
> > IDirectXVideoAccelerationService::CreateSurface().
> >
> > Signed-off-by: Aaron Levinson <alevinsn@aracnet.com>
> > ---
> >  libavutil/hwcontext_dxva2.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavutil/hwcontext_dxva2.c b/libavutil/hwcontext_dxva2.c
> > index 4ed0d56..6c41788 100644
> > --- a/libavutil/hwcontext_dxva2.c
> > +++ b/libavutil/hwcontext_dxva2.c
> > @@ -121,6 +121,13 @@ static void dxva2_frames_uninit(AVHWFramesContext
> *ctx)
> >      }
> >  }
> >
> > +static void dxva2_pool_release_dummy(void *opaque, uint8_t *data)
> > +{
> > +    // important not to free anything here--data is a surface object
> > +    // associated with the call to CreateSurface(), and these surfaces
> are
> > +    // released in dxva2_frames_uninit()
> > +}
> > +
> >  static AVBufferRef *dxva2_pool_alloc(void *opaque, int size)
> >  {
> >      AVHWFramesContext      *ctx = (AVHWFramesContext*)opaque;
> > @@ -130,7 +137,7 @@ static AVBufferRef *dxva2_pool_alloc(void *opaque,
> int size)
> >      if (s->nb_surfaces_used < hwctx->nb_surfaces) {
> >          s->nb_surfaces_used++;
> >          return av_buffer_create((uint8_t*)s->surfaces_internal[s->nb_surfaces_used
> - 1],
> > -                                sizeof(*hwctx->surfaces), NULL, 0, 0);
> > +                                sizeof(*hwctx->surfaces),
> dxva2_pool_release_dummy, 0, 0);
> >      }
> >
> >      return NULL;
>
> LGTM
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

LGTM
Mark Thompson May 16, 2017, 8:54 p.m. UTC | #3
On 16/05/17 13:04, Aaron Levinson wrote:
> Add dxva2_pool_release_dummy() and use it in call to
> av_buffer_create() in dxva2_pool_alloc().
> 
> Prior to this change, av_buffer_create() was called with NULL for the
> third argument, which indicates that av_buffer_default_free() should
> be used to free the buffer's data.  Eventually, it gets to
> buffer_pool_free() and calls buf->free() on a surface object (which is
> av_buffer_default_free()).
> 
> This can result in a crash when the debug version of the C-runtime is
> used on Windows.  While it doesn't appear to result in a crash when
> the release version of the C-runtime is used on Windows, it likely
> results in memory corruption, since av_free() is being called on
> memory that was allocated using
> IDirectXVideoAccelerationService::CreateSurface().
> 
> Signed-off-by: Aaron Levinson <alevinsn@aracnet.com>
> ---
>  libavutil/hwcontext_dxva2.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Applied.

Thanks for finding this - I'm rather surprised we haven't seen more serious problems because of it.

- Mark
diff mbox

Patch

diff --git a/libavutil/hwcontext_dxva2.c b/libavutil/hwcontext_dxva2.c
index 4ed0d56..6c41788 100644
--- a/libavutil/hwcontext_dxva2.c
+++ b/libavutil/hwcontext_dxva2.c
@@ -121,6 +121,13 @@  static void dxva2_frames_uninit(AVHWFramesContext *ctx)
     }
 }
 
+static void dxva2_pool_release_dummy(void *opaque, uint8_t *data)
+{
+    // important not to free anything here--data is a surface object
+    // associated with the call to CreateSurface(), and these surfaces are
+    // released in dxva2_frames_uninit()
+}
+
 static AVBufferRef *dxva2_pool_alloc(void *opaque, int size)
 {
     AVHWFramesContext      *ctx = (AVHWFramesContext*)opaque;
@@ -130,7 +137,7 @@  static AVBufferRef *dxva2_pool_alloc(void *opaque, int size)
     if (s->nb_surfaces_used < hwctx->nb_surfaces) {
         s->nb_surfaces_used++;
         return av_buffer_create((uint8_t*)s->surfaces_internal[s->nb_surfaces_used - 1],
-                                sizeof(*hwctx->surfaces), NULL, 0, 0);
+                                sizeof(*hwctx->surfaces), dxva2_pool_release_dummy, 0, 0);
     }
 
     return NULL;