[FFmpeg-devel] [PATCH 1/2] MxPEG decoder

[Måns Rullgård]

Anatoly Nenashev <anatoly.nenashev at ovsoft.ru> writes:

> On 14.02.2011 14:51, M?ns Rullg?rd wrote:
>> Anatoly Nenashev<anatoly.nenashev at ovsoft.ru>  writes:
>>> +static int mxpeg_decode_mxm(AVCodecContext *avctx, MXpegDecodeContext *mxctx,
>>> +                            char *buf, int len)
>>> +{
>>> +    int32_t  bitmask_size, i;
>>> +    uint32_t mb_count;
>>>      
>> Why do you use sized types here?  For local variables it is almost
>> always better to use plain int or unsigned int types.
>
> I'm not sure that sizeof(int) >= 4 in all architectures supported by
> FFmpeg. Correct me if I wrong.

That is a safe assumption.  If int is smaller than 32 bits nothing will work.

>>> +    av_freep(&mxctx->comment_buffer);
>>> +    mxctx->comment_buffer = buf;
>>> +    mxctx->mxm_bitmask = buf + 12;
>>> +
>>> +    mxctx->mb_width  = AV_RL16(&buf[4]);
>>> +    mxctx->mb_height = AV_RL16(&buf[6]);
>>> +    mb_count = (uint32_t)mxctx->mb_width * mxctx->mb_height;
>>>      
>> Why the cast?
>
> This is also about sizeof(int).
>
>>> +    if (!mb_count || mb_count>  0x7FFFFFF8) {
>>>      
>> This looks strange.  What condition are you actually trying to check for?
>
> This is a check for possible overflow of (mb_count + 7) value.

What happens if it overflows signed 32-bit?  OK, a lot of bad things
probably happen then, but those are independent of this.

-- 
M?ns Rullg?rd
mans at mansr.com