Re: [FFmpeg-devel] [PATCH 1/2] avformat/mov: zero initialize codec_name in mov_parse_stsd_video()

2016-11-12 Thread James Almer 
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 
>> ---
>>  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.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avformat/mov: zero initialize codec_name in mov_parse_stsd_video()

2016-10-20 Thread Andreas Cadhalpun 
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 
> ---
>  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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avformat/mov: zero initialize codec_name in mov_parse_stsd_video()

2016-10-19 Thread Michael Niedermayer 
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 
> >>> ---
> >>>  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


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

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avformat/mov: zero initialize codec_name in mov_parse_stsd_video()

2016-10-19 Thread Andreas Cadhalpun 
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 
>>> ---
>>>  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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avformat/mov: zero initialize codec_name in mov_parse_stsd_video()

2016-10-18 Thread Michael Niedermayer 
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 
> >> ---
> >>  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

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

The real ebay dictionary, page 1
"Used only once"- "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avformat/mov: zero initialize codec_name in mov_parse_stsd_video()

2016-10-18 Thread James Almer 
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 
>> ---
>>  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.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avformat/mov: zero initialize codec_name in mov_parse_stsd_video()

2016-10-17 Thread Michael Niedermayer 
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 
> ---
>  libavformat/mov.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This should be suppressable by adding it to
tests/fate-valgrind.supp

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avformat/mov: zero initialize codec_name in mov_parse_stsd_video()

2016-10-17 Thread James Almer 
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 
>> ---
>>  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.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel