| Message ID | f69648c4-3601-a5c0-a1e5-1361979f5ea7@aracnet.com |
|---|---|
| State | Accepted |
| Headers | show |
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
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
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 --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;
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(-)