Re: [FFmpeg-devel] [FFmpeg-cvslog] cbs_mpeg2: Fix type for marker_bit reading

[Carl Eugen Hoyos]

2017-10-25 10:06 GMT+02:00 Hendrik Leppkes :

> As if static analyzers always care what you can and cannot do. :p

It was actually asan or ubsan iirc.

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

Re: [FFmpeg-devel] [FFmpeg-cvslog] cbs_mpeg2: Fix type for marker_bit reading

[Hendrik Leppkes]

On Wed, Oct 25, 2017 at 12:42 AM, Mark Thompson  wrote:
> On 24/10/17 23:34, Carl Eugen Hoyos wrote:
>> 2017-10-25 0:29 GMT+02:00 Mark Thompson :
>>> On 24/10/17 23:14, Carl Eugen Hoyos wrote:
 2017-10-25 0:09 GMT+02:00 Mark Thompson :
> ffmpeg | branch: master | Mark Thompson  | Tue Oct 24 
> 22:56:48 2017 +0100| [5b2c71bb94d7cab23ee81b5c29388f5fadbcaf22] | 
> committer: Mark Thompson
>
> cbs_mpeg2: Fix type for marker_bit reading
>
>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=5b2c71bb94d7cab23ee81b5c29388f5fadbcaf22
> ---
>
>  libavcodec/cbs_mpeg2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
> index d137762227..0cac29733e 100644
> --- a/libavcodec/cbs_mpeg2.c
> +++ b/libavcodec/cbs_mpeg2.c
> @@ -54,7 +54,7 @@
>  xui(width, name, current->name)
>
>  #define marker_bit() do { \
> -av_unused int one = 1; \
> +av_unused uint32_t one; \
>  CHECK(ff_cbs_read_unsigned(ctx, rw, 1, "marker_bit", , 1, 
> 1)); \

 The commit message doesn't match the change / is this defined behaviour?
>>>
>>> It's only written and never read, so the initialisation isn't doing 
>>> anything.
>>
>> I asked because I believe it was reported once that a static analyzer 
>> protested
>> on passing uninitialized stuff - that was never going to be used - as
>> a parameter.
>> (But that may have been an uninitialized variable that was never used,
>> not a pointer.)
>
> I don't think pointers in general can be complained about in C without 
> knowledge of the target function - consider that there are destination 
> pointers all over the standard library which need not be initialised, like 
> memcpy() or scanf().
>

As if static analyzers always care what you can and cannot do. :p

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

Re: [FFmpeg-devel] [FFmpeg-cvslog] cbs_mpeg2: Fix type for marker_bit reading

[Mark Thompson]

On 24/10/17 23:34, Carl Eugen Hoyos wrote:
> 2017-10-25 0:29 GMT+02:00 Mark Thompson :
>> On 24/10/17 23:14, Carl Eugen Hoyos wrote:
>>> 2017-10-25 0:09 GMT+02:00 Mark Thompson :
 ffmpeg | branch: master | Mark Thompson  | Tue Oct 24 
 22:56:48 2017 +0100| [5b2c71bb94d7cab23ee81b5c29388f5fadbcaf22] | 
 committer: Mark Thompson

 cbs_mpeg2: Fix type for marker_bit reading

> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=5b2c71bb94d7cab23ee81b5c29388f5fadbcaf22
 ---

  libavcodec/cbs_mpeg2.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
 index d137762227..0cac29733e 100644
 --- a/libavcodec/cbs_mpeg2.c
 +++ b/libavcodec/cbs_mpeg2.c
 @@ -54,7 +54,7 @@
  xui(width, name, current->name)

  #define marker_bit() do { \
 -av_unused int one = 1; \
 +av_unused uint32_t one; \
  CHECK(ff_cbs_read_unsigned(ctx, rw, 1, "marker_bit", , 1, 
 1)); \
>>>
>>> The commit message doesn't match the change / is this defined behaviour?
>>
>> It's only written and never read, so the initialisation isn't doing anything.
> 
> I asked because I believe it was reported once that a static analyzer 
> protested
> on passing uninitialized stuff - that was never going to be used - as
> a parameter.
> (But that may have been an uninitialized variable that was never used,
> not a pointer.)

I don't think pointers in general can be complained about in C without 
knowledge of the target function - consider that there are destination pointers 
all over the standard library which need not be initialised, like memcpy() or 
scanf().

If a static analyser looks inside the called function it will find:

"""
int ff_cbs_read_unsigned(CodedBitstreamContext *ctx, GetBitContext *gbc,
 int width, const char *name, uint32_t *write_to,
 uint32_t range_min, uint32_t range_max)
{
... stuff not involving write_to ...

*write_to = value;
return 0;
}
"""

which is I think reasonably clear-cut :)

Thanks,

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

Re: [FFmpeg-devel] [FFmpeg-cvslog] cbs_mpeg2: Fix type for marker_bit reading

[Carl Eugen Hoyos]

2017-10-25 0:29 GMT+02:00 Mark Thompson :
> On 24/10/17 23:14, Carl Eugen Hoyos wrote:
>> 2017-10-25 0:09 GMT+02:00 Mark Thompson :
>>> ffmpeg | branch: master | Mark Thompson  | Tue Oct 24 
>>> 22:56:48 2017 +0100| [5b2c71bb94d7cab23ee81b5c29388f5fadbcaf22] | 
>>> committer: Mark Thompson
>>>
>>> cbs_mpeg2: Fix type for marker_bit reading
>>>
 http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=5b2c71bb94d7cab23ee81b5c29388f5fadbcaf22
>>> ---
>>>
>>>  libavcodec/cbs_mpeg2.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
>>> index d137762227..0cac29733e 100644
>>> --- a/libavcodec/cbs_mpeg2.c
>>> +++ b/libavcodec/cbs_mpeg2.c
>>> @@ -54,7 +54,7 @@
>>>  xui(width, name, current->name)
>>>
>>>  #define marker_bit() do { \
>>> -av_unused int one = 1; \
>>> +av_unused uint32_t one; \
>>>  CHECK(ff_cbs_read_unsigned(ctx, rw, 1, "marker_bit", , 1, 1)); 
>>> \
>>
>> The commit message doesn't match the change / is this defined behaviour?
>
> It's only written and never read, so the initialisation isn't doing anything.

I asked because I believe it was reported once that a static analyzer protested
on passing uninitialized stuff - that was never going to be used - as
a parameter.
(But that may have been an uninitialized variable that was never used,
not a pointer.)

Thank you, Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [FFmpeg-cvslog] cbs_mpeg2: Fix type for marker_bit reading

[Mark Thompson]

On 24/10/17 23:14, Carl Eugen Hoyos wrote:
> 2017-10-25 0:09 GMT+02:00 Mark Thompson :
>> ffmpeg | branch: master | Mark Thompson  | Tue Oct 24 
>> 22:56:48 2017 +0100| [5b2c71bb94d7cab23ee81b5c29388f5fadbcaf22] | committer: 
>> Mark Thompson
>>
>> cbs_mpeg2: Fix type for marker_bit reading
>>
>>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=5b2c71bb94d7cab23ee81b5c29388f5fadbcaf22
>> ---
>>
>>  libavcodec/cbs_mpeg2.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
>> index d137762227..0cac29733e 100644
>> --- a/libavcodec/cbs_mpeg2.c
>> +++ b/libavcodec/cbs_mpeg2.c
>> @@ -54,7 +54,7 @@
>>  xui(width, name, current->name)
>>
>>  #define marker_bit() do { \
>> -av_unused int one = 1; \
>> +av_unused uint32_t one; \
>>  CHECK(ff_cbs_read_unsigned(ctx, rw, 1, "marker_bit", , 1, 1)); \
> 
> The commit message doesn't match the change / is this defined behaviour?

It's only written and never read, so the initialisation isn't doing anything.  
(Hence also the av_unused.)

The type change is fixing a warning on DJGPP (16-bit int is not compatible with 
uint32_t), the other part is a trivial cleanup but should probably still have 
been mentioned.

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