1: Load Image 2: Crash 20070825_2144 ok 20070825_2214 crash http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1188103440&maxdate=1188105239

Instead of checking rowp>rowend, we need to that rowp never extends outside the image data space. Some GIF images just have one additional pixel coming out of the LZW stream.

Instead of adding another field to the gif decoder structure, couldn't you have the OUTPUT_ROW macro do a "if (mGIFStruct.irow >= mGifStruct.height) goto END;"?

That doesn't catch this decoding problem. I already planned to add this check against end of image data to be sure to not overwrite the imagedata, to be safe against any decoding issues. Note, there are still some other optimizations that enable to remove or reduce some members : for example: mGIFStruct.progressive_display = (mGIFStruct.images_decoded == 0) So progressive_display can easily be replaced by that check on images_decoded... "int version" can be replaced by a PRPackedBool (only used in two locations) "int tpixel" can be changed to a PRUint8 (only used in a couple locations, and not directly in image decoding) move "PRUint8 firstchar" near other PRUint8's shuffle sequences of the arrays at the end for better alignment. This could be handled in a separate bug

Tor, this is a much simpler fix to handle ugly GIF's. It appeared that the image ends with an extra LZW block of one byte, and then is followed by a terminator (;). This is not according spec's, but it can be gracefully handled anyway.

Adding magic values (';') to the code for a particular gif image strikes me as the wrong approach.

This check for ';' is done when all data for the GIF image is processed (rows_remaining == 0). What seems to happen here is that these images have as the last lzw block a block of length 1, with one zero byte, and then the Trailer byte (; or 0x3B). In short, instead of the Block Terminator (one zero byte), the image has directly the Trailer byte. So this test is to check when the 'zero' byte is not found but instead the Trailer byte (and all data has been received) then we can consider the image has complete. See also: http://www.w3.org/Graphics/GIF/spec-gif89a.txt 27. Trailer. a. Description. This block is a single-field block indicating the end of the GIF Data Stream. It contains the fixed value 0x3B. b. Required Version. 87a. c. Syntax. 7 6 5 4 3 2 1 0 Field Name Type +---------------+ 0 | | GIF Trailer Byte +---------------+ d. Extensions and Scope. This block does not have scope, it terminates the GIF Data Stream. This block may not be modified by any extension. e. Recommendations. None. Note, in the GIF decoder, characters values like '!', ',' and ';' are used, while the specifications uses hex values like 0x21, 0x2c and 0x3B. To improve readability one could use some defines such as: #define GIF_TRAILER (0x3B) //';' #define GIF_IMAGE_SEPARATOR (0x2C) //',' #define GIF_EXTENSION_INTRODUCER (0x21) //'!' #define GIF_GRAPHIC_CONTROL_LABEL (0xF9) #define GIF_COMMENT_LABEL (0xFE) #define GIF_PLAIN_TEXT_LABEL (0x01) #define GIF_APPLICATION_EXTENSION_LABEL (0xFF)

Comment on attachment 279767 [details] [diff] [review] V2: Simpler fix to catch ill-terminated GIF's Please make another bug/patch to use macros for the field values.

I'd prefer if you used an anonymous enum for the field values; debuggers are much more likely to deal with enums correctly than with macros.

Please request approval1.9 on the attachment... As we're currently in M8 freeze, all check-ins must be approved.

Comment on attachment 280090 [details] [diff] [review] V3: Using 'enum' to define the GIF constants This is a fix for crash on certain GIF images

Checking in modules/libpr0n/decoders/gif/GIF2.h; /cvsroot/mozilla/modules/libpr0n/decoders/gif/GIF2.h,v <-- GIF2.h new revision: 1.26; previous revision: 1.25 done Checking in modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp; /cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp,v <-- nsGIFDecoder2.cpp new revision: 1.83; previous revision: 1.82 done

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091104 Minefield/3.0a8pre. No crash when loading image.