| Message ID | 20171124010155.799-1-jeebjp@gmail.com |
|---|---|
| State | Accepted |
| Commit | f6d49a0dc84aade2adf150c25afb66cbda1d5528 |
| Headers | show |
On Nov 24, 2017 03:01, "Jan Ekström" <jeebjp@gmail.com> wrote: Makes the uninit function re-entrable, which can be a common case when an API user first tries to initialize its context, fails, and then finally unrefs the AVHWDevice. Fixes a crash reported by sm2345 on IRC Relevant backtrace I received from the user: https://pastebin.com/MtWTEgmc Regards, Jan
On Fri, Nov 24, 2017 at 2:01 AM, Jan Ekström <jeebjp@gmail.com> wrote: > Makes the uninit function re-entrable, which can be a common case > when an API user first tries to initialize its context, fails, and > then finally unrefs the AVHWDevice. > > Fixes a crash reported by sm2345 on IRC. > --- > libavutil/hwcontext_d3d11va.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/libavutil/hwcontext_d3d11va.c b/libavutil/hwcontext_d3d11va.c > index 65dd6651fc..845a4a45fe 100644 > --- a/libavutil/hwcontext_d3d11va.c > +++ b/libavutil/hwcontext_d3d11va.c > @@ -458,20 +458,31 @@ static void d3d11va_device_uninit(AVHWDeviceContext *hwdev) > { > AVD3D11VADeviceContext *device_hwctx = hwdev->hwctx; > > - if (device_hwctx->device) > + if (device_hwctx->device) { > ID3D11Device_Release(device_hwctx->device); > + device_hwctx->device = NULL; > + } > > - if (device_hwctx->device_context) > + if (device_hwctx->device_context) { > ID3D11DeviceContext_Release(device_hwctx->device_context); > + device_hwctx->device_context = NULL; > + } > > - if (device_hwctx->video_device) > + if (device_hwctx->video_device) { > ID3D11VideoDevice_Release(device_hwctx->video_device); > + device_hwctx->video_device = NULL; > + } > > - if (device_hwctx->video_context) > + if (device_hwctx->video_context) { > ID3D11VideoContext_Release(device_hwctx->video_context); > + device_hwctx->video_context = NULL; > + } > > - if (device_hwctx->lock == d3d11va_default_lock) > + if (device_hwctx->lock == d3d11va_default_lock) { > CloseHandle(device_hwctx->lock_ctx); > + device_hwctx->lock_ctx = INVALID_HANDLE_VALUE; > + device_hwctx->lock = NULL; > + } > } > LGTM. In some cases the uninit can be called twice, which seems like a bit of an odd design, but we better don't leak any pointers out then. Should probably go through the other hwcontext implementation to check for similar problems. - Hendrik
On 24/11/17 01:01, Jan Ekström wrote: > Makes the uninit function re-entrable, which can be a common case > when an API user first tries to initialize its context, fails, and > then finally unrefs the AVHWDevice. > > Fixes a crash reported by sm2345 on IRC. > --- > libavutil/hwcontext_d3d11va.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/libavutil/hwcontext_d3d11va.c b/libavutil/hwcontext_d3d11va.c > index 65dd6651fc..845a4a45fe 100644 > --- a/libavutil/hwcontext_d3d11va.c > +++ b/libavutil/hwcontext_d3d11va.c > @@ -458,20 +458,31 @@ static void d3d11va_device_uninit(AVHWDeviceContext *hwdev) > { > AVD3D11VADeviceContext *device_hwctx = hwdev->hwctx; > > - if (device_hwctx->device) > + if (device_hwctx->device) { > ID3D11Device_Release(device_hwctx->device); > + device_hwctx->device = NULL; > + } > > - if (device_hwctx->device_context) > + if (device_hwctx->device_context) { > ID3D11DeviceContext_Release(device_hwctx->device_context); > + device_hwctx->device_context = NULL; > + } > > - if (device_hwctx->video_device) > + if (device_hwctx->video_device) { > ID3D11VideoDevice_Release(device_hwctx->video_device); > + device_hwctx->video_device = NULL; > + } > > - if (device_hwctx->video_context) > + if (device_hwctx->video_context) { > ID3D11VideoContext_Release(device_hwctx->video_context); > + device_hwctx->video_context = NULL; > + } > > - if (device_hwctx->lock == d3d11va_default_lock) > + if (device_hwctx->lock == d3d11va_default_lock) { > CloseHandle(device_hwctx->lock_ctx); > + device_hwctx->lock_ctx = INVALID_HANDLE_VALUE; > + device_hwctx->lock = NULL; > + } > } > > static int d3d11va_device_create(AVHWDeviceContext *ctx, const char *device, > LGTM, please apply. I checked the other hwcontext implementations for the same problem, and found it only in OpenCL - it could also crash there, fixed in <http://git.videolan.org/?p=ffmpeg.git;a=commit;h=f4e319d8a94697f21802e2682085599a93a39a57>. Thanks, - Mark
On Sat, Nov 25, 2017 at 6:25 PM, Mark Thompson <sw@jkqxz.net> wrote: > LGTM, please apply. > > I checked the other hwcontext implementations for the same problem, and found it only in OpenCL - it could also crash there, fixed in <http://git.videolan.org/?p=ffmpeg.git;a=commit;h=f4e319d8a94697f21802e2682085599a93a39a57>. > > Thanks, > > - Mark Thanks for the reviews, applied. Jan
diff --git a/libavutil/hwcontext_d3d11va.c b/libavutil/hwcontext_d3d11va.c index 65dd6651fc..845a4a45fe 100644 --- a/libavutil/hwcontext_d3d11va.c +++ b/libavutil/hwcontext_d3d11va.c @@ -458,20 +458,31 @@ static void d3d11va_device_uninit(AVHWDeviceContext *hwdev) { AVD3D11VADeviceContext *device_hwctx = hwdev->hwctx; - if (device_hwctx->device) + if (device_hwctx->device) { ID3D11Device_Release(device_hwctx->device); + device_hwctx->device = NULL; + } - if (device_hwctx->device_context) + if (device_hwctx->device_context) { ID3D11DeviceContext_Release(device_hwctx->device_context); + device_hwctx->device_context = NULL; + } - if (device_hwctx->video_device) + if (device_hwctx->video_device) { ID3D11VideoDevice_Release(device_hwctx->video_device); + device_hwctx->video_device = NULL; + } - if (device_hwctx->video_context) + if (device_hwctx->video_context) { ID3D11VideoContext_Release(device_hwctx->video_context); + device_hwctx->video_context = NULL; + } - if (device_hwctx->lock == d3d11va_default_lock) + if (device_hwctx->lock == d3d11va_default_lock) { CloseHandle(device_hwctx->lock_ctx); + device_hwctx->lock_ctx = INVALID_HANDLE_VALUE; + device_hwctx->lock = NULL; + } } static int d3d11va_device_create(AVHWDeviceContext *ctx, const char *device,