Closed
Bug 209079
Opened 21 years ago
Closed 21 years ago
crash while opening broken GIF image [@ do_lzw]
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
People
(Reporter: aha, Assigned: tor)
References
Details
(Keywords: crash, fixed1.4.1, testcase)
Crash Data
Attachments
(4 files, 3 obsolete files)
11.86 KB,
image/gif
|
Details | |
8.03 KB,
text/plain
|
Details | |
7.18 KB,
text/plain
|
Details | |
719 bytes,
patch
|
pavlov
:
review+
dbaron
:
superreview+
brendan
:
approval1.4.1+
brendan
:
approval1.5b+
|
Details | Diff | Splinter Review |
Opening attached GIF image crash browser - 203061108/trunk/W2K TB20950788Y, TB20950742Q, TB20950741X
Reporter | ||
Comment 1•21 years ago
|
||
Reporter | ||
Comment 2•21 years ago
|
||
BTW it's crashing also 1.4 trunk build -> TB20951536E, TB20951567Q
Severity: normal → critical
Flags: blocking1.4?
Comment 3•21 years ago
|
||
1.4 trunk ? We #have currently a 1.4 branch and a 1.5a trunk
Reporter | ||
Comment 4•21 years ago
|
||
Today 1.4 *branch* build was correct in comment #2.
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.
Lots of things going wrong here, huge numbers and weird pointers.
Comment 7•21 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
Reporter | ||
Updated•21 years ago
|
Flags: blocking1.4?
*** Bug 214075 has been marked as a duplicate of this bug. ***
Summary: crash while opening broken GIF image → crash while opening broken GIF image [@ do_lzw]
Assignee | ||
Comment 12•21 years ago
|
||
Attachment #129288 -
Flags: review?(pavlov)
Updated•21 years ago
|
Attachment #129288 -
Flags: review?(pavlov) → review+
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•21 years ago
|
||
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•21 years ago
|
||
Misunderstood the LZW code - the width limitation isn't needed.
Attachment #129441 -
Flags: superreview?(dbaron)
Attachment #129441 -
Flags: review?(pavlov)
Updated•21 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•21 years ago
|
||
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+
Attachment #129453 -
Flags: review?(pavlov)
Updated•21 years ago
|
Attachment #129453 -
Flags: review?(pavlov) → review+
Comment 18•21 years ago
|
||
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•21 years ago
|
||
Checked in on trunk and 1.4.x branch.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•21 years ago
|
Keywords: fixed1.4.1
Updated•13 years ago
|
Crash Signature: [@ do_lzw]
You need to log in
before you can comment on or make changes to this bug.
Description
•