diff mbox series

[FFmpeg-devel,2/2] avcodec/avdct: Clear IDCTDSPContext context

Message ID 20200127205422.31980-2-michael@niedermayer.cc
State Accepted
Headers show
Series [FFmpeg-devel,1/2] MAINTAINERS: Add patchwork maintainer
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Michael Niedermayer Jan. 27, 2020, 8:54 p.m. UTC
Fixes use of uninitialized variable and segfault

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/avdct.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paul B Mahol Jan. 27, 2020, 8:56 p.m. UTC | #1
lgtm

On 1/27/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> Fixes use of uninitialized variable and segfault
>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/avdct.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/avdct.c b/libavcodec/avdct.c
> index 47e5f7134e..7c761cf39a 100644
> --- a/libavcodec/avdct.c
> +++ b/libavcodec/avdct.c
> @@ -100,7 +100,7 @@ int avcodec_dct_init(AVDCT *dsp)
>
>  #if CONFIG_IDCTDSP
>      {
> -        IDCTDSPContext idsp;
> +        IDCTDSPContext idsp = {0};
>          ff_idctdsp_init(&idsp, avctx);
>          COPY(idsp, idct);
>          COPY(idsp, idct_permutation);
> --
> 2.17.1
>
> _______________________________________________
> 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".
James Almer Jan. 27, 2020, 9:09 p.m. UTC | #2
On 1/27/2020 5:54 PM, Michael Niedermayer wrote:
> Fixes use of uninitialized variable and segfault
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/avdct.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/avdct.c b/libavcodec/avdct.c
> index 47e5f7134e..7c761cf39a 100644
> --- a/libavcodec/avdct.c
> +++ b/libavcodec/avdct.c
> @@ -100,7 +100,7 @@ int avcodec_dct_init(AVDCT *dsp)
>  
>  #if CONFIG_IDCTDSP
>      {
> -        IDCTDSPContext idsp;
> +        IDCTDSPContext idsp = {0};

Should probably be a memset() in ff_idctdsp_init(). This is not the only
IDCTDSPContext user.

>          ff_idctdsp_init(&idsp, avctx);
>          COPY(idsp, idct);
>          COPY(idsp, idct_permutation);
>
Michael Niedermayer Jan. 28, 2020, 12:25 a.m. UTC | #3
On Mon, Jan 27, 2020 at 06:09:28PM -0300, James Almer wrote:
> On 1/27/2020 5:54 PM, Michael Niedermayer wrote:
> > Fixes use of uninitialized variable and segfault
> > 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/avdct.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/avdct.c b/libavcodec/avdct.c
> > index 47e5f7134e..7c761cf39a 100644
> > --- a/libavcodec/avdct.c
> > +++ b/libavcodec/avdct.c
> > @@ -100,7 +100,7 @@ int avcodec_dct_init(AVDCT *dsp)
> >  
> >  #if CONFIG_IDCTDSP
> >      {
> > -        IDCTDSPContext idsp;
> > +        IDCTDSPContext idsp = {0};
> 
> Should probably be a memset() in ff_idctdsp_init(). This is not the only
> IDCTDSPContext user.

this would not work as IDCTDSPContext.mpeg4_studio_profile must be 
initialized but it is also an input to ff_idctdsp_init()

an alternative to the = {0} on the caller side would be to
simply add the mpeg4_studio_profile as an argument to ff_idctdsp_init()
and remove it from the context, its all just internal API so we can
easily redesign this. It should also be documented better ...

What do you suggest ?

thx

[...]
James Almer Jan. 28, 2020, 2:49 a.m. UTC | #4
On 1/27/2020 9:25 PM, Michael Niedermayer wrote:
> On Mon, Jan 27, 2020 at 06:09:28PM -0300, James Almer wrote:
>> On 1/27/2020 5:54 PM, Michael Niedermayer wrote:
>>> Fixes use of uninitialized variable and segfault
>>>
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/avdct.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/avdct.c b/libavcodec/avdct.c
>>> index 47e5f7134e..7c761cf39a 100644
>>> --- a/libavcodec/avdct.c
>>> +++ b/libavcodec/avdct.c
>>> @@ -100,7 +100,7 @@ int avcodec_dct_init(AVDCT *dsp)
>>>  
>>>  #if CONFIG_IDCTDSP
>>>      {
>>> -        IDCTDSPContext idsp;
>>> +        IDCTDSPContext idsp = {0};
>>
>> Should probably be a memset() in ff_idctdsp_init(). This is not the only
>> IDCTDSPContext user.
> 
> this would not work as IDCTDSPContext.mpeg4_studio_profile must be 
> initialized but it is also an input to ff_idctdsp_init()
> 
> an alternative to the = {0} on the caller side would be to
> simply add the mpeg4_studio_profile as an argument to ff_idctdsp_init()
> and remove it from the context, its all just internal API so we can
> easily redesign this. It should also be documented better ...
> 
> What do you suggest ?

memset(c, 0, offsetof(IDCTDSPContext, mpeg4_studio_profile)) in
ff_idctdsp_init() should workaround that without the need to add new
parameters. Just document that mpeg4_studio_profile must be at the end
of the struct, and be the first field of all those that must be
initialized before a call to ff_idctdsp_init(), in case new ones are
added in the future for whatever reason.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>
Michael Niedermayer Jan. 28, 2020, 1:30 p.m. UTC | #5
On Mon, Jan 27, 2020 at 11:49:49PM -0300, James Almer wrote:
> On 1/27/2020 9:25 PM, Michael Niedermayer wrote:
> > On Mon, Jan 27, 2020 at 06:09:28PM -0300, James Almer wrote:
> >> On 1/27/2020 5:54 PM, Michael Niedermayer wrote:
> >>> Fixes use of uninitialized variable and segfault
> >>>
> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>> ---
> >>>  libavcodec/avdct.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavcodec/avdct.c b/libavcodec/avdct.c
> >>> index 47e5f7134e..7c761cf39a 100644
> >>> --- a/libavcodec/avdct.c
> >>> +++ b/libavcodec/avdct.c
> >>> @@ -100,7 +100,7 @@ int avcodec_dct_init(AVDCT *dsp)
> >>>  
> >>>  #if CONFIG_IDCTDSP
> >>>      {
> >>> -        IDCTDSPContext idsp;
> >>> +        IDCTDSPContext idsp = {0};
> >>
> >> Should probably be a memset() in ff_idctdsp_init(). This is not the only
> >> IDCTDSPContext user.
> > 
> > this would not work as IDCTDSPContext.mpeg4_studio_profile must be 
> > initialized but it is also an input to ff_idctdsp_init()
> > 
> > an alternative to the = {0} on the caller side would be to
> > simply add the mpeg4_studio_profile as an argument to ff_idctdsp_init()
> > and remove it from the context, its all just internal API so we can
> > easily redesign this. It should also be documented better ...
> > 
> > What do you suggest ?
> 
> memset(c, 0, offsetof(IDCTDSPContext, mpeg4_studio_profile)) in
> ff_idctdsp_init() should workaround that without the need to add new
> parameters. Just document that mpeg4_studio_profile must be at the end
> of the struct, and be the first field of all those that must be
> initialized before a call to ff_idctdsp_init(), in case new ones are
> added in the future for whatever reason.

The memset() suggested in ff_idctdsp_init() would make no difference
as the function already sets all fields which are not an input.
This would reduce the suggestion to setting mpeg4_studio_profile
in the ?single? caller which does not memset(0) the context prior.

That leaves 3 suggestions
A. "-        IDCTDSPContext idsp;"
A. "+        IDCTDSPContext idsp = {0};"     (for one caller)

B. "+        idsp.mpeg4_studio_profile = 0;" (for one caller)

C. "-        ff_idctdsp_init();"   
C. "+        ff_idctdsp_init(0);"            (for all callers)

In all cases ff_idctdsp_init() also probably should be documented.

What do you prefer ?

thx

[...]
James Almer Jan. 31, 2020, 3:47 p.m. UTC | #6
On 1/28/2020 10:30 AM, Michael Niedermayer wrote:
> On Mon, Jan 27, 2020 at 11:49:49PM -0300, James Almer wrote:
>> On 1/27/2020 9:25 PM, Michael Niedermayer wrote:
>>> On Mon, Jan 27, 2020 at 06:09:28PM -0300, James Almer wrote:
>>>> On 1/27/2020 5:54 PM, Michael Niedermayer wrote:
>>>>> Fixes use of uninitialized variable and segfault
>>>>>
>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>> ---
>>>>>  libavcodec/avdct.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavcodec/avdct.c b/libavcodec/avdct.c
>>>>> index 47e5f7134e..7c761cf39a 100644
>>>>> --- a/libavcodec/avdct.c
>>>>> +++ b/libavcodec/avdct.c
>>>>> @@ -100,7 +100,7 @@ int avcodec_dct_init(AVDCT *dsp)
>>>>>  
>>>>>  #if CONFIG_IDCTDSP
>>>>>      {
>>>>> -        IDCTDSPContext idsp;
>>>>> +        IDCTDSPContext idsp = {0};
>>>>
>>>> Should probably be a memset() in ff_idctdsp_init(). This is not the only
>>>> IDCTDSPContext user.
>>>
>>> this would not work as IDCTDSPContext.mpeg4_studio_profile must be 
>>> initialized but it is also an input to ff_idctdsp_init()
>>>
>>> an alternative to the = {0} on the caller side would be to
>>> simply add the mpeg4_studio_profile as an argument to ff_idctdsp_init()
>>> and remove it from the context, its all just internal API so we can
>>> easily redesign this. It should also be documented better ...
>>>
>>> What do you suggest ?
>>
>> memset(c, 0, offsetof(IDCTDSPContext, mpeg4_studio_profile)) in
>> ff_idctdsp_init() should workaround that without the need to add new
>> parameters. Just document that mpeg4_studio_profile must be at the end
>> of the struct, and be the first field of all those that must be
>> initialized before a call to ff_idctdsp_init(), in case new ones are
>> added in the future for whatever reason.
> 
> The memset() suggested in ff_idctdsp_init() would make no difference
> as the function already sets all fields which are not an input.
> This would reduce the suggestion to setting mpeg4_studio_profile
> in the ?single? caller which does not memset(0) the context prior.
> 
> That leaves 3 suggestions
> A. "-        IDCTDSPContext idsp;"
> A. "+        IDCTDSPContext idsp = {0};"     (for one caller)
> 
> B. "+        idsp.mpeg4_studio_profile = 0;" (for one caller)
> 
> C. "-        ff_idctdsp_init();"   
> C. "+        ff_idctdsp_init(0);"            (for all callers)
> 
> In all cases ff_idctdsp_init() also probably should be documented.
> 
> What do you prefer ?

In that case just go with A, your original patch.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>
Michael Niedermayer Jan. 31, 2020, 9:17 p.m. UTC | #7
On Fri, Jan 31, 2020 at 12:47:57PM -0300, James Almer wrote:
> On 1/28/2020 10:30 AM, Michael Niedermayer wrote:
> > On Mon, Jan 27, 2020 at 11:49:49PM -0300, James Almer wrote:
> >> On 1/27/2020 9:25 PM, Michael Niedermayer wrote:
> >>> On Mon, Jan 27, 2020 at 06:09:28PM -0300, James Almer wrote:
> >>>> On 1/27/2020 5:54 PM, Michael Niedermayer wrote:
> >>>>> Fixes use of uninitialized variable and segfault
> >>>>>
> >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>>> ---
> >>>>>  libavcodec/avdct.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/libavcodec/avdct.c b/libavcodec/avdct.c
> >>>>> index 47e5f7134e..7c761cf39a 100644
> >>>>> --- a/libavcodec/avdct.c
> >>>>> +++ b/libavcodec/avdct.c
> >>>>> @@ -100,7 +100,7 @@ int avcodec_dct_init(AVDCT *dsp)
> >>>>>  
> >>>>>  #if CONFIG_IDCTDSP
> >>>>>      {
> >>>>> -        IDCTDSPContext idsp;
> >>>>> +        IDCTDSPContext idsp = {0};
> >>>>
> >>>> Should probably be a memset() in ff_idctdsp_init(). This is not the only
> >>>> IDCTDSPContext user.
> >>>
> >>> this would not work as IDCTDSPContext.mpeg4_studio_profile must be 
> >>> initialized but it is also an input to ff_idctdsp_init()
> >>>
> >>> an alternative to the = {0} on the caller side would be to
> >>> simply add the mpeg4_studio_profile as an argument to ff_idctdsp_init()
> >>> and remove it from the context, its all just internal API so we can
> >>> easily redesign this. It should also be documented better ...
> >>>
> >>> What do you suggest ?
> >>
> >> memset(c, 0, offsetof(IDCTDSPContext, mpeg4_studio_profile)) in
> >> ff_idctdsp_init() should workaround that without the need to add new
> >> parameters. Just document that mpeg4_studio_profile must be at the end
> >> of the struct, and be the first field of all those that must be
> >> initialized before a call to ff_idctdsp_init(), in case new ones are
> >> added in the future for whatever reason.
> > 
> > The memset() suggested in ff_idctdsp_init() would make no difference
> > as the function already sets all fields which are not an input.
> > This would reduce the suggestion to setting mpeg4_studio_profile
> > in the ?single? caller which does not memset(0) the context prior.
> > 
> > That leaves 3 suggestions
> > A. "-        IDCTDSPContext idsp;"
> > A. "+        IDCTDSPContext idsp = {0};"     (for one caller)
> > 
> > B. "+        idsp.mpeg4_studio_profile = 0;" (for one caller)
> > 
> > C. "-        ff_idctdsp_init();"   
> > C. "+        ff_idctdsp_init(0);"            (for all callers)
> > 
> > In all cases ff_idctdsp_init() also probably should be documented.
> > 
> > What do you prefer ?
> 
> In that case just go with A, your original patch.

ok, will apply

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/avdct.c b/libavcodec/avdct.c
index 47e5f7134e..7c761cf39a 100644
--- a/libavcodec/avdct.c
+++ b/libavcodec/avdct.c
@@ -100,7 +100,7 @@  int avcodec_dct_init(AVDCT *dsp)
 
 #if CONFIG_IDCTDSP
     {
-        IDCTDSPContext idsp;
+        IDCTDSPContext idsp = {0};
         ff_idctdsp_init(&idsp, avctx);
         COPY(idsp, idct);
         COPY(idsp, idct_permutation);