[FFmpeg-devel] Fix crash if av_vdpau_bind_context() is not used.

Submitted by Ivan Kalvachev on Oct. 9, 2017, 12:04 a.m.

Details

Message ID CABA=pqdZE2=THZiDdcL7DhGBBq6R_wr8mWXvmQDskyF8vhsNtA@mail.gmail.com
State New
Headers show

Commit Message

Ivan Kalvachev Oct. 9, 2017, 12:04 a.m.
The public functions av_alloc_vdpaucontext() and
av_vdpau_alloc_context() are allocating AVVDPAUContext
structure that is supposed to be placed in avctx->hwaccel_context.

However the rest of libavcodec/vdpau.c uses avctx->hwaccel_context
as struct VDPAUHWContext, that is bigger and does contain
AVVDPAUContext as first member.

The usage includes write to the new variables in the bigger stuct,
without checking for block size.

Fix by always allocating the bigger structure.

BTW,
I have no idea why the new fields haven't simply been added to the
existing struct...
It seems that the programmer who wrote this has been aware of the problem,
because av_vdpau_bind_context reallocates the structure.

It might be good idea to check the other usages of this reallocation function.

Best Regards
   Ivan Kalvachev

Comments

wm4 Oct. 9, 2017, 11:30 a.m.
On Mon, 9 Oct 2017 03:04:53 +0300
Ivan Kalvachev <ikalvachev@gmail.com> wrote:

> The public functions av_alloc_vdpaucontext() and
> av_vdpau_alloc_context() are allocating AVVDPAUContext
> structure that is supposed to be placed in avctx->hwaccel_context.
> 
> However the rest of libavcodec/vdpau.c uses avctx->hwaccel_context
> as struct VDPAUHWContext, that is bigger and does contain
> AVVDPAUContext as first member.
> 
> The usage includes write to the new variables in the bigger stuct,
> without checking for block size.
> 
> Fix by always allocating the bigger structure.
> 
> BTW,
> I have no idea why the new fields haven't simply been added to the
> existing struct...
> It seems that the programmer who wrote this has been aware of the problem,
> because av_vdpau_bind_context reallocates the structure.
> 
> It might be good idea to check the other usages of this reallocation function.
> 
> Best Regards
>    Ivan Kalvachev

IMO not really worth fixing at this point, because this is the old-old
vdpau API. Even av_vdpau_bind_context() (which does not require using
av_alloc_vdpaucontext()) is deprecated. Or rather should be - I just
haven't bothered deprecating it because the deprecation dance is too
messy. In any case, you shouldn't use any of those APIs - use the
generic hwaccel API instead (setting hw_frames_ctx or hw_device_ctx).
Ivan Kalvachev Oct. 10, 2017, 12:24 a.m.
On 10/9/17, wm4 <nfxjfg@googlemail.com> wrote:
> On Mon, 9 Oct 2017 03:04:53 +0300
> Ivan Kalvachev <ikalvachev@gmail.com> wrote:
>
>> The public functions av_alloc_vdpaucontext() and
>> av_vdpau_alloc_context() are allocating AVVDPAUContext
>> structure that is supposed to be placed in avctx->hwaccel_context.
>>
>> However the rest of libavcodec/vdpau.c uses avctx->hwaccel_context
>> as struct VDPAUHWContext, that is bigger and does contain
>> AVVDPAUContext as first member.
>>
>> The usage includes write to the new variables in the bigger stuct,
>> without checking for block size.
>>
>> Fix by always allocating the bigger structure.
>>
>> BTW,
>> I have no idea why the new fields haven't simply been added to the
>> existing struct...
>> It seems that the programmer who wrote this has been aware of the problem,
>> because av_vdpau_bind_context reallocates the structure.
>>
>> It might be good idea to check the other usages of this reallocation
>> function.
>>
>> Best Regards
>>    Ivan Kalvachev
>
> IMO not really worth fixing at this point, because this is the old-old
> vdpau API. Even av_vdpau_bind_context() (which does not require using
> av_alloc_vdpaucontext()) is deprecated. Or rather should be - I just
> haven't bothered deprecating it because the deprecation dance is too
> messy. In any case, you shouldn't use any of those APIs - use the
> generic hwaccel API instead (setting hw_frames_ctx or hw_device_ctx).

Every bug must be fixed, even if the code is going to be removed next.

Since you "didn't bother" to deprecate it, this code will remain even after
the API bump. And it is still (mis)used by at least one program that
crashed on me.

So it MUST be fixed.

Feel free at any time to mark it as deprecated
and set a deprecation target.


Best Regards
   Ivan Kalvachev
James Almer Oct. 10, 2017, 1:27 a.m.
On 10/9/2017 8:30 AM, wm4 wrote:
> On Mon, 9 Oct 2017 03:04:53 +0300
> Ivan Kalvachev <ikalvachev@gmail.com> wrote:
> 
>> The public functions av_alloc_vdpaucontext() and
>> av_vdpau_alloc_context() are allocating AVVDPAUContext
>> structure that is supposed to be placed in avctx->hwaccel_context.
>>
>> However the rest of libavcodec/vdpau.c uses avctx->hwaccel_context
>> as struct VDPAUHWContext, that is bigger and does contain
>> AVVDPAUContext as first member.
>>
>> The usage includes write to the new variables in the bigger stuct,
>> without checking for block size.
>>
>> Fix by always allocating the bigger structure.
>>
>> BTW,
>> I have no idea why the new fields haven't simply been added to the
>> existing struct...
>> It seems that the programmer who wrote this has been aware of the problem,
>> because av_vdpau_bind_context reallocates the structure.
>>
>> It might be good idea to check the other usages of this reallocation function.
>>
>> Best Regards
>>    Ivan Kalvachev
> 
> IMO not really worth fixing at this point, because this is the old-old
> vdpau API. Even av_vdpau_bind_context() (which does not require using
> av_alloc_vdpaucontext()) is deprecated. Or rather should be - I just
> haven't bothered deprecating it because the deprecation dance is too
> messy. In any case, you shouldn't use any of those APIs - use the
> generic hwaccel API instead (setting hw_frames_ctx or hw_device_ctx).

av_vdpau_alloc_context() is not deprecated, neither here on in libav.
APIChanges only has a mention that av_vdpau_bind_context() should be
used instead and that's it. It doesn't even seem to mention that neither
should be used at all in favor of the hwaccel API.

If all these are truly deprecated, then I'd recommend doing the dance
asap to the clock may start ticking.
wm4 Oct. 10, 2017, 11:28 a.m.
On Tue, 10 Oct 2017 03:24:56 +0300
Ivan Kalvachev <ikalvachev@gmail.com> wrote:

> On 10/9/17, wm4 <nfxjfg@googlemail.com> wrote:
> > On Mon, 9 Oct 2017 03:04:53 +0300
> > Ivan Kalvachev <ikalvachev@gmail.com> wrote:
> >  
> >> The public functions av_alloc_vdpaucontext() and
> >> av_vdpau_alloc_context() are allocating AVVDPAUContext
> >> structure that is supposed to be placed in avctx->hwaccel_context.
> >>
> >> However the rest of libavcodec/vdpau.c uses avctx->hwaccel_context
> >> as struct VDPAUHWContext, that is bigger and does contain
> >> AVVDPAUContext as first member.
> >>
> >> The usage includes write to the new variables in the bigger stuct,
> >> without checking for block size.
> >>
> >> Fix by always allocating the bigger structure.
> >>
> >> BTW,
> >> I have no idea why the new fields haven't simply been added to the
> >> existing struct...
> >> It seems that the programmer who wrote this has been aware of the problem,
> >> because av_vdpau_bind_context reallocates the structure.
> >>
> >> It might be good idea to check the other usages of this reallocation
> >> function.
> >>
> >> Best Regards
> >>    Ivan Kalvachev  
> >
> > IMO not really worth fixing at this point, because this is the old-old
> > vdpau API. Even av_vdpau_bind_context() (which does not require using
> > av_alloc_vdpaucontext()) is deprecated. Or rather should be - I just
> > haven't bothered deprecating it because the deprecation dance is too
> > messy. In any case, you shouldn't use any of those APIs - use the
> > generic hwaccel API instead (setting hw_frames_ctx or hw_device_ctx).  
> 
> Every bug must be fixed, even if the code is going to be removed next.
> 
> Since you "didn't bother" to deprecate it, this code will remain even after
> the API bump. And it is still (mis)used by at least one program that
> crashed on me.
> 
> So it MUST be fixed.

Well, if you insist, feel free to attempt to maintain it, but I won't
care, even if I make changes to the code under the newer API (about
which I care). I tried to remove some vdpau code earlier which had
been deprecated for years, but this was rejected, so why care about
deprecation or whether an ancient API is actually broken? Not me.
Ivan Kalvachev Oct. 11, 2017, 2:45 p.m.
On 10/10/17, wm4 <nfxjfg@googlemail.com> wrote:
> On Tue, 10 Oct 2017 03:24:56 +0300
> Ivan Kalvachev <ikalvachev@gmail.com> wrote:
>
>> On 10/9/17, wm4 <nfxjfg@googlemail.com> wrote:
>> > On Mon, 9 Oct 2017 03:04:53 +0300
>> > Ivan Kalvachev <ikalvachev@gmail.com> wrote:
>> >
>> >> The public functions av_alloc_vdpaucontext() and
>> >> av_vdpau_alloc_context() are allocating AVVDPAUContext
>> >> structure that is supposed to be placed in avctx->hwaccel_context.
>> >>
>> >> However the rest of libavcodec/vdpau.c uses avctx->hwaccel_context
>> >> as struct VDPAUHWContext, that is bigger and does contain
>> >> AVVDPAUContext as first member.
>> >>
>> >> The usage includes write to the new variables in the bigger stuct,
>> >> without checking for block size.
>> >>
>> >> Fix by always allocating the bigger structure.
>> >>
>> >> BTW,
>> >> I have no idea why the new fields haven't simply been added to the
>> >> existing struct...
>> >> It seems that the programmer who wrote this has been aware of the
>> >> problem,
>> >> because av_vdpau_bind_context reallocates the structure.
>> >>
>> >> It might be good idea to check the other usages of this reallocation
>> >> function.
>> >>
>> >> Best Regards
>> >>    Ivan Kalvachev
>> >
>> > IMO not really worth fixing at this point, because this is the old-old
>> > vdpau API. Even av_vdpau_bind_context() (which does not require using
>> > av_alloc_vdpaucontext()) is deprecated. Or rather should be - I just
>> > haven't bothered deprecating it because the deprecation dance is too
>> > messy. In any case, you shouldn't use any of those APIs - use the
>> > generic hwaccel API instead (setting hw_frames_ctx or hw_device_ctx).
>>
>> Every bug must be fixed, even if the code is going to be removed next.
>>
>> Since you "didn't bother" to deprecate it, this code will remain even
>> after
>> the API bump. And it is still (mis)used by at least one program that
>> crashed on me.
>>
>> So it MUST be fixed.
>
> Well, if you insist, feel free to attempt to maintain it, but I won't
> care, even if I make changes to the code under the newer API (about
> which I care). I tried to remove some vdpau code earlier which had
> been deprecated for years, but this was rejected, so why care about
> deprecation or whether an ancient API is actually broken? Not me.

You are not making any sense.

Anyway. Do you have any objection for this patch to be committed?
Carl Eugen Hoyos Oct. 12, 2017, 10:16 p.m.
2017-10-09 2:04 GMT+02:00 Ivan Kalvachev <ikalvachev@gmail.com>:
> The public functions av_alloc_vdpaucontext() and
> av_vdpau_alloc_context() are allocating AVVDPAUContext
> structure that is supposed to be placed in avctx->hwaccel_context.
>
> However the rest of libavcodec/vdpau.c uses avctx->hwaccel_context
> as struct VDPAUHWContext, that is bigger and does contain
> AVVDPAUContext as first member.
>
> The usage includes write to the new variables in the bigger stuct,
> without checking for block size.
>
> Fix by always allocating the bigger structure.

Patch applied and backported.

Thank you, Carl Eugen
Ivan Kalvachev Oct. 12, 2017, 10:16 p.m.
On 10/11/17, Ivan Kalvachev <ikalvachev@gmail.com> wrote:
> On 10/10/17, wm4 <nfxjfg@googlemail.com> wrote:
>> On Tue, 10 Oct 2017 03:24:56 +0300
>> Ivan Kalvachev <ikalvachev@gmail.com> wrote:
>>
>>> On 10/9/17, wm4 <nfxjfg@googlemail.com> wrote:
>>> > On Mon, 9 Oct 2017 03:04:53 +0300
>>> > Ivan Kalvachev <ikalvachev@gmail.com> wrote:
>>> >
>>> >> The public functions av_alloc_vdpaucontext() and
>>> >> av_vdpau_alloc_context() are allocating AVVDPAUContext
>>> >> structure that is supposed to be placed in avctx->hwaccel_context.
>>> >>
>>> >> However the rest of libavcodec/vdpau.c uses avctx->hwaccel_context
>>> >> as struct VDPAUHWContext, that is bigger and does contain
>>> >> AVVDPAUContext as first member.
>>> >>
>>> >> The usage includes write to the new variables in the bigger stuct,
>>> >> without checking for block size.
>>> >>
>>> >> Fix by always allocating the bigger structure.
>>> >>
>>> >> BTW,
>>> >> I have no idea why the new fields haven't simply been added to the
>>> >> existing struct...
>>> >> It seems that the programmer who wrote this has been aware of the
>>> >> problem,
>>> >> because av_vdpau_bind_context reallocates the structure.
>>> >>
>>> >> It might be good idea to check the other usages of this reallocation
>>> >> function.
>>> >>
>>> >> Best Regards
>>> >>    Ivan Kalvachev
>>> >
>>> > IMO not really worth fixing at this point, because this is the old-old
>>> > vdpau API. Even av_vdpau_bind_context() (which does not require using
>>> > av_alloc_vdpaucontext()) is deprecated. Or rather should be - I just
>>> > haven't bothered deprecating it because the deprecation dance is too
>>> > messy. In any case, you shouldn't use any of those APIs - use the
>>> > generic hwaccel API instead (setting hw_frames_ctx or hw_device_ctx).
>>>
>>> Every bug must be fixed, even if the code is going to be removed next.
>>>
>>> Since you "didn't bother" to deprecate it, this code will remain even
>>> after
>>> the API bump. And it is still (mis)used by at least one program that
>>> crashed on me.
>>>
>>> So it MUST be fixed.
>>
>> Well, if you insist, feel free to attempt to maintain it, but I won't
>> care, even if I make changes to the code under the newer API (about
>> which I care). I tried to remove some vdpau code earlier which had
>> been deprecated for years, but this was rejected, so why care about
>> deprecation or whether an ancient API is actually broken? Not me.
>
> You are not making any sense.
>
> Anyway. Do you have any objection for this patch to be committed?

I'd like this fix to be committed before the 3.4 release.
Anybody?
Ivan Kalvachev Oct. 12, 2017, 10:22 p.m.
On 10/13/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2017-10-09 2:04 GMT+02:00 Ivan Kalvachev <ikalvachev@gmail.com>:
>> The public functions av_alloc_vdpaucontext() and
>> av_vdpau_alloc_context() are allocating AVVDPAUContext
>> structure that is supposed to be placed in avctx->hwaccel_context.
>>
>> However the rest of libavcodec/vdpau.c uses avctx->hwaccel_context
>> as struct VDPAUHWContext, that is bigger and does contain
>> AVVDPAUContext as first member.
>>
>> The usage includes write to the new variables in the bigger stuct,
>> without checking for block size.
>>
>> Fix by always allocating the bigger structure.
>
> Patch applied and backported.
>
> Thank you, Carl Eugen

Thank you.

Patch hide | download patch | download mbox

From c9dafbf5402ebf8c68bf8648ecea7a74282113a8 Mon Sep 17 00:00:00 2001
From: Ivan Kalvachev <ikalvachev@gmail.com>
Date: Mon, 9 Oct 2017 02:40:26 +0300
Subject: [PATCH] Fix crash if av_vdpau_bind_context() is not used.

The public functions av_alloc_vdpaucontext() and
av_vdpau_alloc_context() are allocating AVVDPAUContext
structure that is supposed to be placed in avctx->hwaccel_context.

However the rest of libavcodec/vdpau.c uses avctx->hwaccel_context
as struct VDPAUHWContext, that is bigger and does contain
AVVDPAUContext as first member.

The usage includes write to the new variables in the bigger stuct,
without checking for block size.

Fix by always allocating the bigger structure.

Signed-off-by: Ivan Kalvachev <ikalvachev@gmail.com>
---
 libavcodec/vdpau.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/vdpau.c b/libavcodec/vdpau.c
index 42ebddbee..4cc51cb79 100644
--- a/libavcodec/vdpau.c
+++ b/libavcodec/vdpau.c
@@ -816,7 +816,7 @@  do {                                       \
 
 AVVDPAUContext *av_vdpau_alloc_context(void)
 {
-    return av_mallocz(sizeof(AVVDPAUContext));
+    return av_mallocz(sizeof(VDPAUHWContext));
 }
 
 int av_vdpau_bind_context(AVCodecContext *avctx, VdpDevice device,
-- 
2.14.1