| Message ID | 20170611140725.11936-1-timo@rothenpieler.org |
|---|---|
| State | New |
| Headers | show |
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.)
> 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.
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
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];