[FFmpeg-devel] [PATCH] gifdec: use truncated width for image manipulation

[Michael Niedermayer]

On Sun, Aug 17, 2014 at 09:47:25PM +0200, Christophe Gisquet wrote:
> Hi,
> 
> 2014-08-17 20:39 GMT+02:00 Michael Niedermayer <michaelni at gmx.at>:
> >> +    if (width > s->screen_width) {
> >> +        av_log(s->avctx, AV_LOG_ERROR, "Invalid image width.\n");
> >> +        return AVERROR_INVALIDDATA;
> >> +    }
> >> +    if (left + width > s->screen_width) {
> >> +        /* width must be kept around to avoid lzw vs line desync */
> >> +        pw = s->screen_width - left;
> >> +        av_log(s->avctx, AV_LOG_WARNING, "Image too wide by %d, truncating.\n",
> >> +               left + width - s->screen_width);
> >> +    } else {
> >> +        pw = width;
> >> +    }
> >> +    if (top + height > s->screen_height) {
> >> +        /* we don't care about the extra invisible lines */
> >> +        av_log(s->avctx, AV_LOG_WARNING, "Image too high by %d, truncating.\n",
> >> +               top + height - s->screen_height);
> >> +        height = s->screen_height - top;
> >> +    }
> >
> > i think these need a check for top >= s->screen_height and
> > left >= s->screen_width
> 
> Because of integer wraparound/overflow/... and/or values being
> potentially negative? If yes, I don't think it can happen:
>     left   = bytestream2_get_le16u(&s->gb);
>     top    = bytestream2_get_le16u(&s->gb);
>     width  = bytestream2_get_le16u(&s->gb);
>     height = bytestream2_get_le16u(&s->gb);
> 
> And the conditions are then already part of the new checks, right?

i tried to set top to 0xFFFF and decoding a few random files i got

0x00000000007319ea in gif_fill_rect (picture=0x1a96a60, color=16777215, l=0, t=65535, w=192, h=-65367) at libavcodec/gifdec.c:108
108                 *px = color;
(gdb) bt
#0  0x00000000007319ea in gif_fill_rect (picture=0x1a96a60, color=16777215, l=0, t=65535, w=192, h=-65367) at libavcodec/gifdec.c:108
#1  0x0000000000731e84 in gif_read_image (s=0x1a88160, frame=0x1a96a60) at libavcodec/gifdec.c:208
#2  0x0000000000732780 in gif_parse_next_image (s=0x1a88160, frame=0x1a96a60) at libavcodec/gifdec.c:432
#3  0x0000000000732af8 in gif_decode_frame (avctx=0x1a87620, data=0x1a8b5e0, got_frame=0x7fffffffdd18, avpkt=0x7fffffffda20) at libavcodec/gifdec.c:514
#4  0x0000000000a59cc8 in avcodec_decode_video2 (avctx=0x1a87620, picture=0x1a8b5e0, got_picture_ptr=0x7fffffffdd18, avpkt=0x7fffffffdc60) at libavcodec/utils.c:2264
#5  0x000000000042db73 in decode_video (ist=0x1a87420, pkt=0x7fffffffdc60, got_output=0x7fffffffdd18) at ffmpeg.c:1888
#6  0x000000000042ebe3 in process_input_packet (ist=0x1a87420, pkt=0x7fffffffdde0) at ffmpeg.c:2122
#7  0x00000000004351da in process_input (file_index=0) at ffmpeg.c:3529
#8  0x000000000043556f in transcode_step () at ffmpeg.c:3623
#9  0x000000000043567c in transcode () at ffmpeg.c:3675
#10 0x0000000000435b5c in main (argc=6, argv=0x7fffffffe3d8) at ffmpeg.c:3851

it doesnt seem to occur under valgrind

i didnt investigate why this happens

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140817/8a8487e1/attachment.asc>