394403 - Crash Viewing Image [@ nsGIFDecoder2::DoLzw]

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Brian Polidoro]

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

Comment 1

[Brian Polidoro]

Attachment: problem gif

Comment 2

[Brian Polidoro]

bp-c5b22c73-576d-11dc-a632-001a4bd43ed6

Comment 3

[Alfred Kayser]

Attachment: V1: Better checking to not write image pixels outside image

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.

Comment 4

[tor]

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;"?

Comment 5

[Alfred Kayser]

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

Comment 6

[Alfred Kayser]

Attachment: V2: Simpler fix to catch ill-terminated GIF's

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.

Comment 7

[tor]

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

Comment 8

[Alfred Kayser]

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 9

[tor]

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.

Comment 10

[Jeff Walden [:Waldo]]

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.

Comment 11

[Alfred Kayser]

Attachment: V3: Using 'enum' to define the GIF constants

Comment 12

[Reed Loden [:reed]]

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

Comment 13

[Alfred Kayser]

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

Comment 14

[Reed Loden [:reed]]

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

Comment 15

[Marcia Knous [:marcia]]

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.