Last Comment Bug 209079 - crash while opening broken GIF image [@ do_lzw]
: crash while opening broken GIF image [@ do_lzw]
Status: RESOLVED FIXED
: crash, fixed1.4.1, testcase
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- critical with 3 votes (vote)
: ---
Assigned To: tor
: Terri Preston
Mentors:
: 214075 (view as bug list)
Depends on:
Blocks: 224532
  Show dependency treegraph
 
Reported: 2003-06-11 12:55 PDT by Adam Hauner
Modified: 2003-11-03 01:47 PST (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
crashing GIF file (11.86 KB, image/gif)
2003-06-11 12:56 PDT, Adam Hauner
no flags Details
Linux stack trace (8.03 KB, text/plain)
2003-06-11 14:54 PDT, tenthumbs
no flags Details
os x stack trace (7.18 KB, text/plain)
2003-06-11 19:23 PDT, Chris Casciano
no flags Details
make sure we don't blow out the lzw stack (1.18 KB, patch)
2003-08-06 09:10 PDT, tor
pavlov: review+
dbaron: superreview-
Details | Diff | Review
modify locations of stack checking (1.21 KB, patch)
2003-08-08 07:27 PDT, tor
no flags Details | Diff | Review
minimize checking, remove width limit (503 bytes, patch)
2003-08-08 08:18 PDT, tor
pavlov: review+
dbaron: superreview+
Details | Diff | Review
add back another stack check (719 bytes, patch)
2003-08-08 11:37 PDT, tor
pavlov: review+
dbaron: superreview+
brendan: approval1.4.1+
brendan: approval1.5b+
Details | Diff | Review

Description Adam Hauner 2003-06-11 12:55:33 PDT
Opening attached GIF image crash browser - 203061108/trunk/W2K

TB20950788Y, TB20950742Q, TB20950741X
Comment 1 Adam Hauner 2003-06-11 12:56:24 PDT
Created attachment 125442 [details]
crashing GIF file
Comment 2 Adam Hauner 2003-06-11 13:30:17 PDT
BTW it's crashing also 1.4 trunk build -> TB20951536E, TB20951567Q
Comment 3 Matthias Versen [:Matti] 2003-06-11 13:54:42 PDT
1.4 trunk ?
We #have currently a 1.4 branch and a 1.5a trunk
Comment 4 Adam Hauner 2003-06-11 14:04:38 PDT
Today 1.4 *branch* build was correct in comment #2.
Comment 5 tenthumbs 2003-06-11 14:52:24 PDT
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 2003-06-11 14:54:47 PDT
Created attachment 125454 [details]
Linux stack trace

Lots of things going wrong here, huge numbers and weird pointers.
Comment 7 Chris Casciano 2003-06-11 19:22:36 PDT
-> 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 2003-06-11 19:23:59 PDT
Created attachment 125484 [details]
os x stack trace

full stack mentioned in comment 7
Comment 9 Tim 2003-07-28 09:23:08 PDT
*** Bug 214075 has been marked as a duplicate of this bug. ***
Comment 10 timeless 2003-07-28 13:32:55 PDT
.
Comment 11 tor 2003-08-06 09:09:55 PDT
Taking bug.
Comment 12 tor 2003-08-06 09:10:58 PDT
Created attachment 129288 [details] [diff] [review]
make sure we don't blow out the lzw stack
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-08-06 14:27:37 PDT
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 2003-08-08 07:27:29 PDT
Created attachment 129439 [details] [diff] [review]
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 2003-08-08 08:18:42 PDT
Created attachment 129441 [details] [diff] [review]
minimize checking, remove width limit

Misunderstood the LZW code - the width limitation isn't needed.
Comment 16 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-08-08 10:38:45 PDT
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 2003-08-08 11:37:10 PDT
Created attachment 129453 [details] [diff] [review]
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] 2003-08-20 10:28:10 PDT
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 2003-08-20 11:07:23 PDT
Checked in on trunk and 1.4.x branch.

Note You need to log in before you can comment on or make changes to this bug.