209079 - crash while opening broken GIF image [@ do_lzw]

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Adam Hauner]

Opening attached GIF image crash browser - 203061108/trunk/W2K

TB20950788Y, TB20950742Q, TB20950741X

Comment 1

[Adam Hauner]

Attachment: crashing GIF file

Comment 2

[Adam Hauner]

BTW it's crashing also 1.4 trunk build -> TB20951536E, TB20951567Q

Comment 3

[Matthias Versen [:Matti]]

1.4 trunk ?
We #have currently a 1.4 branch and a 1.5a trunk

Comment 4

[Adam Hauner]

Today 1.4 *branch* build was correct in comment #2.

Comment 5

[tenthumbs]

Crashes my Linux trunk build. Gifsicle says

* servers.gif 1 image
  logical screen 8342x8357
  global color table [256]
  background 32
  + image #0 8342x8357 at 8224,8224 interlaced

so it's a huge image at a huge offset. If this actually worked mozilla 
would need a 16566x16581 image. Converted to RGB that's at least 786M.

Comment 6

[tenthumbs]

Attachment: Linux stack trace

Lots of things going wrong here, huge numbers and weird pointers.

Comment 7

[Chris Casciano]

-> All/All (i'm seeing this on OS X as well.

Crashed thread from Camino 2003061104 here the relevant thread... full log being
attached in a sec:

**********

Date/Time:  2003-06-11 22:18:40 -0400
OS Version: 10.2.6 (Build 6L60)
Host:       pnhTiObject.local.

Command:    Camino
PID:        447

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0x05e74000

Thread 0 Crashed:
 #0   0x001d9254 in do_lzw(gif_struct*, unsigned char const*)
 #1   0x001d9518 in gif_write(gif_struct*, unsigned char const*, unsigned)
 #2   0x001df344 in nsGIFDecoder2::ProcessData(unsigned char*, unsigned, unsigned*)
 #3   0x001df154 in ReadDataOut(nsIInputStream*, void*, char const*, unsigned,
unsigned, unsigned*)
 #4   0x0502b428 in nsInputStreamTee::WriteSegmentFun(nsIInputStream*, void*,
char const*, unsigned, unsigned, unsigned*)
 #5   0x0502e92c in nsPipeInputStream::ReadSegments(unsigned
(*)(nsIInputStream*, void*, char const*, unsigned, unsigned, unsigned*), void*,
unsigned, unsigned*)
 #6   0x004d90f4 in imgRequest::OnDataAvailable(nsIRequest*, nsISupports*,
nsIInputStream*, unsigned, unsigned)
 #7   0x001146cc in nsDocumentOpenInfo::OnDataAvailable(nsIRequest*,
nsISupports*, nsIInputStream*, unsigned, unsigned)
 #8   0x001901f0 in nsStreamListenerTee::OnDataAvailable(nsIRequest*,
nsISupports*, nsIInputStream*, unsigned, unsigned)
 #9   0x00471854 in nsHttpChannel::OnDataAvailable(nsIRequest*, nsISupports*,
nsIInputStream*, unsigned, unsigned)
 #10  0x0017d5dc in nsInputStreamPump::OnStateTransfer()
 #11  0x0017d394 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*)
 #12  0x05091638 in nsInputStreamReadyEvent::EventHandler(PLEvent*)
 #13  0x05045de8 in PL_HandleEvent
 #14  0x05045d0c in PL_ProcessPendingEvents
 #15  0x05046ca4 in nsEventQueueImpl::ProcessPendingEvents()
 #16  0x00407190 in -[EventQueueHandler eventTimer:]
 #17  0x907e521c in __NSFireTimer
 #18  0x90163230 in __CFRunLoopDoTimer
 #19  0x90148d28 in __CFRunLoopRun
 #20  0x90180f58 in CFRunLoopRunSpecific
 #21  0x969a3b70 in RunCurrentEventLoopInMode
 #22  0x969b3a78 in ReceiveNextEventCommon
 #23  0x969dabbc in BlockUntilNextEventMatchingListInMode
 #24  0x9308dedc in _DPSNextEvent
 #25  0x930a0158 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
 #26  0x930b1d88 in -[NSApplication run]
 #27  0x9315fc58 in NSApplicationMain
 #28  0x000050e0 in _start
 #29  0x00004f60 in start

Comment 8

[Chris Casciano]

Attachment: os x stack trace

full stack mentioned in comment 7

Comment 9

[Tim]

*** Bug 214075 has been marked as a duplicate of this bug. ***

Comment 10

[timeless]

.

Comment 11

[tor]

Taking bug.

Comment 12

[tor]

Attachment: make sure we don't blow out the lzw stack

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 129288 [details] [diff] [review]
make sure we don't blow out the lzw stack

>+
>+        if (stackp == gs->stack + MAX_BITS)
>+          return -1;
>       }

There's a local copy of |gs->stack| in the variable |stack|.  Use that instead
here, and in the next change.


>       *stackp++ = firstchar = suffix[code];
>+      if (stackp == gs->stack + MAX_BITS)
>+        return -1;
> 
>       /* Define a new codeword in the dictionary. */
>       if (avail < 4096) {

Do you need this change?  It looks to me as though stackp will not be
incremented again before it's decremented, so this is just forbidding a width
that exactly fits (which you allow below).

>@@ -846,6 +853,13 @@
>           gs->state = gif_error;
>           break;
>         }
>+      }
>+
>+      /* make sure the width won't exceed the lzw stack size */
>+      if (width > MAX_BITS)
>+      {
>+        gs->state = gif_error;
>+        break;
>       }
> 
>       gs->height = height;

Is this limit of 4097 defined in the GIF spec or is it something arbitrary that
we're imposing?  If the latter, couldn't we allocate buffers of variable size
to fix this?

Comment 14

[tor]

Attachment: modify locations of stack checking

The width limitation is decoder imposed, as the spec allows a 16-bit unsigned
value.	I didn't modify the stack allocation to be dynamic because the limit
seemed large enough and I didn't want to give nsRecyclingAllocator a hard time.

Comment 15

[tor]

Attachment: minimize checking, remove width limit

Misunderstood the LZW code - the width limitation isn't needed.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 129441 [details] [diff] [review]
minimize checking, remove width limit

It looks like you've removed the first check as well.  That makes sense
assuming that a precondition of do_lzw is that gs->stack == gs->stackp.  Is
that the case?	If so, do you want to add an assertion to that effect?

Other than that, sr=dbaron, although I'm somewhat curious what types of images
trigger this.  Does the spec allow arbitrarily large images, and they can still
avoid this code, unless they're malformed?

Comment 17

[tor]

Attachment: add back another stack check

I think that would be an appropriate clear condition if it wasn't for the
clear code.  The GIF spec allows images with 16-bit unsigned dimensions.
gs->stack is used for reversing the pattern found in the LZW dictionary,
which should be a bounded length.

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 129453 [details] [diff] [review]
add back another stack check

Looks good for trunk and 1.4.x branch.

/be

Comment 19

[tor]

Checked in on trunk and 1.4.x branch.