[FFmpeg-devel,10/11] avcodec/h264_parser: zero-initialize H264PredWeightTable

Submitted by Timo Rothenpieler on June 11, 2017, 2:07 p.m.

Details

Message ID 20170611140725.11936-1-timo@rothenpieler.org
State New
Headers show

Commit Message

Timo Rothenpieler June 11, 2017, 2:07 p.m.
Fixes CID 1404889
---
 libavcodec/h264_parser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mark Thompson June 11, 2017, 2:48 p.m.
On 11/06/17 15:07, Timo Rothenpieler wrote:
> Fixes CID 1404889
> ---
>  libavcodec/h264_parser.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
> index 2564c6c6c3..1a304f318f 100644
> --- a/libavcodec/h264_parser.c
> +++ b/libavcodec/h264_parser.c
> @@ -155,7 +155,7 @@ found:
>  static int scan_mmco_reset(AVCodecParserContext *s, GetBitContext *gb,
>                             void *logctx)
>  {
> -    H264PredWeightTable pwt;
> +    H264PredWeightTable pwt = { 0 };
>      int slice_type_nos = s->pict_type & 3;
>      H264ParseContext *p = s->priv_data;
>      int list_count, ref_count[2];
> 

Seems dubious?  That is not a small structure, and it's being used essentially write-only here to skip over an unwanted part of the slice header - since it will only ever write to a small proportion of the elements, initialising all of them to zero feels like a waste.

(The only argument in Coverity seems to be that passing a pointer to an uninitialised structure to an external function is bad - it hasn't actually looked at the function to observe that it doesn't read anything currently in the structure.)
Timo Rothenpieler June 11, 2017, 3:12 p.m.
> Seems dubious?  That is not a small structure, and it's being used essentially write-only here to skip over an unwanted part of the slice header - since it will only ever write to a small proportion of the elements, initialising all of them to zero feels like a waste.
> 
> (The only argument in Coverity seems to be that passing a pointer to an uninitialised structure to an external function is bad - it hasn't actually looked at the function to observe that it doesn't read anything currently in the structure.)

It calls ff_h264_pred_weight_table in line 204, which it does analyze, 
and which accesses pwt->chroma_log2_weight_denom, which was not 
initialized before.
I don't think doing = { 0 }; is expensive. iirc all elements except for 
the first one are zero-initialized already, even though it's 
implementation-defined or even undefined.
Mark Thompson June 11, 2017, 3:59 p.m.
On 11/06/17 16:12, Timo Rothenpieler wrote:
>> Seems dubious?  That is not a small structure, and it's being used essentially write-only here to skip over an unwanted part of the slice header - since it will only ever write to a small proportion of the elements, initialising all of them to zero feels like a waste.
>>
>> (The only argument in Coverity seems to be that passing a pointer to an uninitialised structure to an external function is bad - it hasn't actually looked at the function to observe that it doesn't read anything currently in the structure.)
> 
> It calls ff_h264_pred_weight_table in line 204, which it does analyze, and which accesses pwt->chroma_log2_weight_denom, which was not initialized before.

I think those accesses are wrong and should be fixed - patch following.

(I could believe that the monochrome H.264 stream with prediction weights required to make this actually error out with the current code is quite rare.)

> I don't think doing = { 0 }; is expensive. iirc all elements except for the first one are zero-initialized already, even though it's implementation-defined or even undefined.

It's all indeterminate - see C11 section 6.7.9.  A compiler could in theory choose to initialise it to zero or 42 or any other value, but there is no reason for any particular choice so it won't bother.

- Mark

Patch hide | download patch | download mbox

diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
index 2564c6c6c3..1a304f318f 100644
--- a/libavcodec/h264_parser.c
+++ b/libavcodec/h264_parser.c
@@ -155,7 +155,7 @@  found:
 static int scan_mmco_reset(AVCodecParserContext *s, GetBitContext *gb,
                            void *logctx)
 {
-    H264PredWeightTable pwt;
+    H264PredWeightTable pwt = { 0 };
     int slice_type_nos = s->pict_type & 3;
     H264ParseContext *p = s->priv_data;
     int list_count, ref_count[2];