The default bug view has changed. See this FAQ.

crash while opening broken GIF image [@ do_lzw]

RESOLVED FIXED

Status

()

Core
ImageLib
--
critical
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Adam Hauner, Assigned: tor)

Tracking

({crash, fixed1.4.1, testcase})

Trunk
crash, fixed1.4.1, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

14 years ago
Opening attached GIF image crash browser - 203061108/trunk/W2K

TB20950788Y, TB20950742Q, TB20950741X
(Reporter)

Comment 1

14 years ago
Created attachment 125442 [details]
crashing GIF file
(Reporter)

Comment 2

14 years ago
BTW it's crashing also 1.4 trunk build -> TB20951536E, TB20951567Q
Severity: normal → critical
Flags: blocking1.4?
1.4 trunk ?
We #have currently a 1.4 branch and a 1.5a trunk
(Reporter)

Comment 4

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

Comment 5

14 years ago
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

14 years ago
Created attachment 125454 [details]
Linux stack trace

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

Comment 7

14 years ago
-> 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
Keywords: stackwanted
OS: Windows 2000 → All
Hardware: PC → All

Comment 8

14 years ago
Created attachment 125484 [details]
os x stack trace

full stack mentioned in comment 7
(Reporter)

Updated

14 years ago
Flags: blocking1.4?

Comment 9

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

Updated

14 years ago
Summary: crash while opening broken GIF image → crash while opening broken GIF image [@ do_lzw]

Comment 10

14 years ago
.
Assignee: jdunn → paper
(Assignee)

Comment 11

14 years ago
Taking bug.
Assignee: paper → tor
(Assignee)

Comment 12

14 years ago
Created attachment 129288 [details] [diff] [review]
make sure we don't blow out the lzw stack
(Assignee)

Updated

14 years ago
Attachment #129288 - Flags: review?(pavlov)

Updated

14 years ago
Attachment #129288 - Flags: review?(pavlov) → review+
(Assignee)

Updated

14 years ago
Attachment #129288 - Flags: superreview?(dbaron)
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?
(Assignee)

Comment 14

14 years ago
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.
Attachment #129288 - Attachment is obsolete: true
(Assignee)

Comment 15

14 years ago
Created attachment 129441 [details] [diff] [review]
minimize checking, remove width limit

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

Updated

14 years ago
Attachment #129441 - Flags: superreview?(dbaron)
Attachment #129441 - Flags: review?(pavlov)

Updated

14 years ago
Attachment #129441 - Flags: review?(pavlov) → review+
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?
Attachment #129441 - Flags: superreview?(dbaron) → superreview+
Attachment #129288 - Flags: superreview?(dbaron) → superreview-
(Assignee)

Comment 17

14 years ago
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.
Attachment #129439 - Attachment is obsolete: true
Attachment #129441 - Attachment is obsolete: true
Attachment #129453 - Flags: superreview+
(Assignee)

Updated

14 years ago
Attachment #129453 - Flags: review?(pavlov)

Updated

14 years ago
Attachment #129453 - Flags: review?(pavlov) → review+
Comment on attachment 129453 [details] [diff] [review]
add back another stack check

Looks good for trunk and 1.4.x branch.

/be
Attachment #129453 - Flags: approval1.5b+
Attachment #129453 - Flags: approval1.4.x+
(Assignee)

Comment 19

14 years ago
Checked in on trunk and 1.4.x branch.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Keywords: fixed1.4.1

Updated

14 years ago
Blocks: 224532
Crash Signature: [@ do_lzw]
You need to log in before you can comment on or make changes to this bug.