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

[Michael Niedermayer]

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 at 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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If the United States is serious about tackling the national security threats 
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200131/3308d4cc/attachment.sig>