[FFmpeg-devel] [PATCH] Fix buffer overflow checks to avoid integer overflows.

[Reimar Döffinger]

On 29 Mar 2011, at 23:38, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Tue, Mar 29, 2011 at 09:15:33PM +0200, Reimar Döffinger wrote:
>> ---
>> libavcodec/dfa.c |   41 ++++++++++++++++++++++-------------------
>> 1 files changed, 22 insertions(+), 19 deletions(-)
>> 
>> diff --git a/libavcodec/dfa.c b/libavcodec/dfa.c
>> index 6736234..26ca984 100644
>> --- a/libavcodec/dfa.c
>> +++ b/libavcodec/dfa.c
>> @@ -62,12 +62,14 @@ static int decode_tsw1(uint8_t *frame, int width, int height,
>>     const uint8_t *frame_start = frame;
>>     const uint8_t *frame_end   = frame + width * height;
>>     int mask = 0x10000, bitbuf = 0;
>> -    int v, offset, count, segments;
>> +    int v, count, segments;
>> +    unsigned offset;
>> 
>>     segments = bytestream_get_le32(&src);
>> -    frame   += bytestream_get_le32(&src);
>> -    if (frame < frame_start || frame > frame_end)
>> +    offset   = bytestream_get_le32(&src);
>> +    if (frame_end - frame <= offset)
> 
> the condition changes from < to <=, is this intended?

Yes, for == we'd be at the end and there's nothing to write. It seems at least safer to not allow it to continue there.

> [...]
>> @@ -232,15 +234,16 @@ static int decode_wdlt(uint8_t *frame, int width, int height,
>>     int count, i, v, lines, segments;
>> 
>>     lines = bytestream_get_le16(&src);
>> -    if (frame + lines * width > frame_end || src >= src_end)
>> +    if (lines > height || src >= src_end)
>>         return -1;
>> 
>>     while (lines--) {
>>         segments = bytestream_get_le16(&src);
>>         while ((segments & 0xC000) == 0xC000) {
>> -            frame    -= (int16_t)segments * width;
>> -            if (frame >= frame_end)
>> +            int delta = -((int16_t)segments * width);
> 
> can the multiplication here overflow?

I don't think so, but it seems a bad idea to assume it.
The type should be unsigned, just be sure.