| Message ID | 20161017003451.8152-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | 66453b1fba6c68f2f7f5117355d34b5f40910327 |
| Headers | show |
Hi, On 17/10/2016 02:34, James Almer wrote: > Fixes valgrind warning about "Conditional jump or move depends on uninitialised value(s)" > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/mov.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index add1812..7462ecf 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -1802,7 +1802,7 @@ static int mov_codec_id(AVStream *st, uint32_t format) > static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb, > AVStream *st, MOVStreamContext *sc) > { > - uint8_t codec_name[32]; > + uint8_t codec_name[32] = { 0 }; > int64_t stsd_start; > unsigned int len; > Do we really need to "fix" false positive from Valgrind?
On 10/17/2016 10:05 AM, Benoit Fouet wrote: > Hi, > > > On 17/10/2016 02:34, James Almer wrote: >> Fixes valgrind warning about "Conditional jump or move depends on uninitialised value(s)" >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/mov.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/mov.c b/libavformat/mov.c >> index add1812..7462ecf 100644 >> --- a/libavformat/mov.c >> +++ b/libavformat/mov.c >> @@ -1802,7 +1802,7 @@ static int mov_codec_id(AVStream *st, uint32_t format) >> static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb, >> AVStream *st, MOVStreamContext *sc) >> { >> - uint8_t codec_name[32]; >> + uint8_t codec_name[32] = { 0 }; >> int64_t stsd_start; >> unsigned int len; >> > > Do we really need to "fix" false positive from Valgrind? I don't see why not. It's a one line change that zero initializes stack and removes noise from the valgrind fate clients, making actual memleaks and such in the future easy to notice after a quick glance.
On Sun, Oct 16, 2016 at 09:34:50PM -0300, James Almer wrote: > Fixes valgrind warning about "Conditional jump or move depends on uninitialised value(s)" > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/mov.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) This should be suppressable by adding it to tests/fate-valgrind.supp [...]
On 10/17/2016 9:57 PM, Michael Niedermayer wrote: > On Sun, Oct 16, 2016 at 09:34:50PM -0300, James Almer wrote: >> Fixes valgrind warning about "Conditional jump or move depends on uninitialised value(s)" >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/mov.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > This should be suppressable by adding it to > tests/fate-valgrind.supp I'll leave that to someone else. No idea how to add stuff to that file. Should i drop this patch? Zero initializing a buffer in stack wouldn't hurt IMO.
On Tue, Oct 18, 2016 at 11:15:50PM -0300, James Almer wrote: > On 10/17/2016 9:57 PM, Michael Niedermayer wrote: > > On Sun, Oct 16, 2016 at 09:34:50PM -0300, James Almer wrote: > >> Fixes valgrind warning about "Conditional jump or move depends on uninitialised value(s)" > >> > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> libavformat/mov.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > > This should be suppressable by adding it to > > tests/fate-valgrind.supp > > I'll leave that to someone else. No idea how to add stuff to that file. IIRC, valgrind can print the proper formatted supperssion, so should only require copy and paste from that > > Should i drop this patch? Zero initializing a buffer in stack wouldn't > hurt IMO. iam not objecting to the zero init [...]
On 19.10.2016 04:15, James Almer wrote: > On 10/17/2016 9:57 PM, Michael Niedermayer wrote: >> On Sun, Oct 16, 2016 at 09:34:50PM -0300, James Almer wrote: >>> Fixes valgrind warning about "Conditional jump or move depends on uninitialised value(s)" >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavformat/mov.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> This should be suppressable by adding it to >> tests/fate-valgrind.supp > > I'll leave that to someone else. No idea how to add stuff to that file. > > Should i drop this patch? Zero initializing a buffer in stack wouldn't > hurt IMO. I prefer fixing such things in the code if it's reasonable possible, which is the case here. In other words, the patch looks good to me. Best regards, Andreas
On Wed, Oct 19, 2016 at 07:30:29PM +0200, Andreas Cadhalpun wrote: > On 19.10.2016 04:15, James Almer wrote: > > On 10/17/2016 9:57 PM, Michael Niedermayer wrote: > >> On Sun, Oct 16, 2016 at 09:34:50PM -0300, James Almer wrote: > >>> Fixes valgrind warning about "Conditional jump or move depends on uninitialised value(s)" > >>> > >>> Signed-off-by: James Almer <jamrial@gmail.com> > >>> --- > >>> libavformat/mov.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> This should be suppressable by adding it to > >> tests/fate-valgrind.supp > > > > I'll leave that to someone else. No idea how to add stuff to that file. > > > > Should i drop this patch? Zero initializing a buffer in stack wouldn't > > hurt IMO. > > I prefer fixing such things in the code if it's reasonable possible, which > is the case here. In other words, the patch looks good to me. I think the bug is that valgrind misinterprets highly optimized libc/gcc code, i might be wrong though as i didnt disassemble and analyze this, that was just my feeling ... But if true a change to ffmpeg can only workaround but not fix this [...]
On 20.10.2016 03:25, Michael Niedermayer wrote: > On Wed, Oct 19, 2016 at 07:30:29PM +0200, Andreas Cadhalpun wrote: >> On 19.10.2016 04:15, James Almer wrote: >>> On 10/17/2016 9:57 PM, Michael Niedermayer wrote: >>>> On Sun, Oct 16, 2016 at 09:34:50PM -0300, James Almer wrote: >>>>> Fixes valgrind warning about "Conditional jump or move depends on uninitialised value(s)" >>>>> >>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>> --- >>>>> libavformat/mov.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> This should be suppressable by adding it to >>>> tests/fate-valgrind.supp >>> >>> I'll leave that to someone else. No idea how to add stuff to that file. >>> >>> Should i drop this patch? Zero initializing a buffer in stack wouldn't >>> hurt IMO. >> >> I prefer fixing such things in the code if it's reasonable possible, which >> is the case here. In other words, the patch looks good to me. > > I think the bug is that valgrind misinterprets highly optimized libc/gcc > code, i might be wrong though as i didnt disassemble and analyze this, > that was just my feeling ... > But if true a change to ffmpeg can only workaround but not fix this Yes, this seems to be a false positive. But working around it is good, because it increases the signal to noise ratio of valgrind. This is similar to false positive compiler warnings: Not working around them has high chances of wasting the time of the next one to look into it. Best regards, Andreas
On 10/20/2016 3:25 PM, Andreas Cadhalpun wrote: > On 20.10.2016 03:25, Michael Niedermayer wrote: >> On Wed, Oct 19, 2016 at 07:30:29PM +0200, Andreas Cadhalpun wrote: >>> On 19.10.2016 04:15, James Almer wrote: >>>> On 10/17/2016 9:57 PM, Michael Niedermayer wrote: >>>>> On Sun, Oct 16, 2016 at 09:34:50PM -0300, James Almer wrote: >>>>>> Fixes valgrind warning about "Conditional jump or move depends on uninitialised value(s)" >>>>>> >>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>> --- >>>>>> libavformat/mov.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> This should be suppressable by adding it to >>>>> tests/fate-valgrind.supp >>>> >>>> I'll leave that to someone else. No idea how to add stuff to that file. >>>> >>>> Should i drop this patch? Zero initializing a buffer in stack wouldn't >>>> hurt IMO. >>> >>> I prefer fixing such things in the code if it's reasonable possible, which >>> is the case here. In other words, the patch looks good to me. >> >> I think the bug is that valgrind misinterprets highly optimized libc/gcc >> code, i might be wrong though as i didnt disassemble and analyze this, >> that was just my feeling ... >> But if true a change to ffmpeg can only workaround but not fix this > > Yes, this seems to be a false positive. But working around it is good, because > it increases the signal to noise ratio of valgrind. > This is similar to false positive compiler warnings: Not working around them > has high chances of wasting the time of the next one to look into it. > > Best regards, > Andreas Pushed. Three tests were failing now because of this, up from one when i first submitted this patch, so even as a workaround it still gets rid of the noise and will make new actual leaks or overreads easily noticeable. Anyone wanting to fix this by adding a suppression or by fixing Valgrind is welcome.
diff --git a/libavformat/mov.c b/libavformat/mov.c index add1812..7462ecf 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -1802,7 +1802,7 @@ static int mov_codec_id(AVStream *st, uint32_t format) static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb, AVStream *st, MOVStreamContext *sc) { - uint8_t codec_name[32]; + uint8_t codec_name[32] = { 0 }; int64_t stsd_start; unsigned int len;
Fixes valgrind warning about "Conditional jump or move depends on uninitialised value(s)" Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/mov.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)