| Message ID | 20200127205422.31980-2-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/2] MAINTAINERS: Add patchwork maintainer | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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".
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); >
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 [...]
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". >
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 [...]
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". >
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 --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);
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(-)