204994 - crash if i view the attachment (attached to bug 196977)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[:glob ✱]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030504 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030504 Mozilla Firebird/0.6

attachment 116918 [details] on bug 196977 is a bmp that crashed firebird.

Reproducible: Always

Steps to Reproduce:
1. http://bugzilla.mozilla.org/attachment.cgi?id=116918&action=view

Actual Results:  
crash

Expected Results:  
viewed the bmp, or shown it as corrupt.

IE shows the bmp as corrupt.  not many apps can view that bmp correctly.

TB19928957E
TB19929076Y

Comment 1

[noririty]

confirming

-> Browser
-> critical
-> ImageLib?

Comment 2

[Erik]

Well, when viewing the first time it WFM using Gecko/20030504 Mozilla
Firebird/0.6 on both Win2k and WinXP, but the second time I tried both crashed.
Though there's something magic with this bmp Firebird shouldn't crash - that's
for sure...

Comment 3

[jag (Peter Annema)]

msvcrt.dll + 0x335d1 (0x77c435d1)
nsBMPDecoder::ReadSegCb
[d:/nightlybuild/phoenix/trunk/mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp,
line 139]
nsInputStreamTee::WriteSegmentFun
[d:/nightlybuild/phoenix/trunk/mozilla/xpcom/io/nsInputStreamTee.cpp, line 106]
nsPipeInputStream::ReadSegments
[d:/nightlybuild/phoenix/trunk/mozilla/xpcom/io/nsPipe3.cpp, line 733]
nsInputStreamTee::ReadSegments
[d:/nightlybuild/phoenix/trunk/mozilla/xpcom/io/nsInputStreamTee.cpp, line 160]
nsBMPDecoder::WriteFrom
[d:/nightlybuild/phoenix/trunk/mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp,
line 146]
imgRequest::OnDataAvailable
[d:/nightlybuild/phoenix/trunk/mozilla/modules/libpr0n/src/imgRequest.cpp, line 794]
ProxyListener::OnDataAvailable
[d:/nightlybuild/phoenix/trunk/mozilla/modules/libpr0n/src/imgLoader.cpp, line 873]
nsMediaDocumentStreamListener::OnDataAvailable
[d:/nightlybuild/phoenix/trunk/mozilla/content/html/document/src/nsMediaDocument.cpp,
line 112]
nsDocumentOpenInfo::OnDataAvailable
[d:/nightlybuild/phoenix/trunk/mozilla/uriloader/base/nsURILoader.cpp, line 241]
nsStreamListenerTee::OnDataAvailable
[d:/nightlybuild/phoenix/trunk/mozilla/netwerk/base/src/nsStreamListenerTee.cpp,
line 98]
nsHttpChannel::OnDataAvailable
[d:/nightlybuild/phoenix/trunk/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,
line 3175]
nsInputStreamPump::OnStateTransfer
[d:/nightlybuild/phoenix/trunk/mozilla/netwerk/base/src/nsInputStreamPump.cpp,
line 421]
nsInputStreamPump::OnInputStreamReady
[d:/nightlybuild/phoenix/trunk/mozilla/netwerk/base/src/nsInputStreamPump.cpp,
line 322]
nsInputStreamReadyEvent::EventHandler
[d:/nightlybuild/phoenix/trunk/mozilla/xpcom/io/nsStreamUtils.cpp, line 117]
PL_HandleEvent [d:/nightlybuild/phoenix/trunk/mozilla/xpcom/threads/plevent.c,
line 660]
PL_ProcessPendingEvents
[d:/nightlybuild/phoenix/trunk/mozilla/xpcom/threads/plevent.c, line 596]
_md_EventReceiverProc
[d:/nightlybuild/phoenix/trunk/mozilla/xpcom/threads/plevent.c, line 1396]
USER32.dll + 0x4455 (0x77d44455)
USER32.dll + 0x95d5 (0x77d495d5)
nsAppShellService::Run
[d:/nightlybuild/phoenix/trunk/mozilla/xpfe/appshell/src/nsAppShellService.cpp,
line 479]
main1 [d:/nightlybuild/phoenix/trunk/mozilla/toolkit/xre/nsAppRunner.cpp, line 1297]
xre_main [d:/nightlybuild/phoenix/trunk/mozilla/toolkit/xre/nsAppRunner.cpp,
line 1698]
main [d:/nightlybuild/phoenix/trunk/mozilla/browser/app/nsBrowserApp.cpp, line 52]
WinMain [d:/nightlybuild/phoenix/trunk/mozilla/browser/app/nsBrowserApp.cpp,
line 65]
WinMainCRTStartup()
kernel32.dll + 0x214c7 (0x77e814c7) 

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

as a note, I don't seem to crash on linux/mozilla with a build from yesterday

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

er, nevermind me, I do crash here.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

taking bug

Comment 7

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4b) Gecko/20030508 crashes always,
never triggering Talkback. Can paste info, if wanted.
Crashed twice after load, when I tried to reload, and once, when about 75% was
loaded, only top 25% were lacking.

Premiere, my first Talkback from Firebird!

TB19935324M

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4b) Gecko/20030504 Mozilla Firebird/0.6

And Win98SE says: (invalid page)

PHOENIX verursachte einen Fehler durch eine ungültige Seite
in Modul NSPR4.DLL bei 0167:60143a71.
Register:
EAX=ffffffff CS=0167 EIP=60143a71 EFLGS=00010286
EBX=00000050 SS=016f ESP=0179cebc EBP=00d9e6c8
ECX=00f07080 DS=016f ESI=00d9e6c6 FS=4c07
EDX=00000000 ES=016f EDI=00000000 GS=5abe
Bytes bei CS:EIP:
3b 50 14 74 1b 8b 40 08 85 c0 75 f4 85 c9 8b c1 
Stapelwerte:
60159ec5 00f07080 00000000 00d9e6b8 00000002 00000000 ffffffff 00000000 00000002
00000000 00000001 00000000 00d9e6c8 00000000 00000000 00000000 

Comment 8

[Hermann Schwab]

Always same address, this time even before starting to draw, instantly after
clicking on URL of testcase. After i acknowledged that, there came a similar
alarm from NECKO.DLL, and I had to cancel some more alarms, without looking at
them, and then restarted mozilla for this posting.

BuildID 2003050804, Win98SE, nForce grafic, 800x600


MOZILLA verursachte einen Fehler durch eine ungültige Seite
in Modul MSVCRT.DLL bei 0167:780019ef.
Register:
EAX=ffffffff CS=0167 EIP=780019ef EFLGS=00010216
EBX=0288bc78 SS=016f ESP=0065f820 EBP=0065f84c
ECX=3ffeef1e DS=016f ESI=028b2e50 FS=3627
EDX=00000000 ES=016f EDI=028d0000 GS=216f
Bytes bei CS:EIP:
f3 ab 85 d2 0f 85 2b ff 00 00 8b 44 24 08 5f c3 
Stapelwerte:
028b2f04 6145a3c7 0288bc9f 000000ff ffffffd9 0065f8b4 028b45d0 80000000 61e792b6
00000002 61e449f7 0065f884 61459b1e 028b2e01 01b98170 00000078 

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

people,
there is no need to continue trying to reproduce this bug.

I can reproduce it locally, and am working on a fix.

Comment 10

[tenthumbs]

It's interesting that I only crash on Linux if I open the image from bugzilla.
If I copy the BMP to my own server I never crash.

Comment 11

[Greg K.]

WorksForMe using FizzillaMach/2003-05-07-14-trunk (1.4b) on Mac OS X. "Image
contains errors" message shown.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

this bug has two underlying issues....

.) the bmp decoder does not check the validity of input data at one point (this
causes the crash)
.) the bmp decoder seems to do something wrong when rle images that are loaded
come in several packets, at least sometimes

I fixed the first one but the second one must be addressed as well

Comment 13

[tenthumbs]

> I fixed the first one

So what's the fix?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

the fix is the following, but I will wait with seeking review until I have a fix
for the other one too.
Index: nsBMPDecoder.cpp
===================================================================
RCS file: /cvsroot/mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp,v
retrieving revision 1.21
diff -u -7 -p -r1.21 nsBMPDecoder.cpp
--- nsBMPDecoder.cpp    26 Feb 2003 05:51:16 -0000      1.21
+++ nsBMPDecoder.cpp    10 May 2003 15:57:30 -0000
@@ -584,14 +597,16 @@ NS_METHOD nsBMPDecoder::ProcessData(cons
                         break;
  
                     case eRLEStateNeedXDelta:
                         // Handle the XDelta and proceed to get Y Delta
                         byte = *aBuffer++;
                         aCount--;
                         mAlphaPtr += byte;
+                        if (mAlphaPtr > mAlpha + mBIH.width)
+                            mAlphaPtr = mAlpha + mBIH.width;
                         mDecoding += byte * GFXBYTESPERPIXEL;
  
                         mState = eRLEStateNeedYDelta;
                         continue;
  
                     case eRLEStateNeedYDelta:
                         // Get the Y Delta and then "handle" the move

Comment 15

[tenthumbs]

Without this patch my 04-10 Linux trunk build will not crash if I set a
breakpoint and stop on it. When I continue the full image appears. With the
patch I get either the bottom half of the image or 

###!!! ASSERTION: writer returned an error with non-zero writeCount:
'(NS_FAILED(rv) ? (*writeCount == 0) : PR_TRUE)', file nsInputStreamTee.cpp,
line 108

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 122966 [details] [diff] [review]
patch

this patch fixes the crash in the bmp decoder and a few related problems,
mostly in the RLE code.

there were not sufficient checks if enough input data is there and if the input
data is valid

the aBuffer&1 comparison is wrong because an earlier ProcessData call might
have had an odd byte count

Comment 18

[[e-mail]]

Comment on attachment 122966 [details] [diff] [review]
patch

r=me because it does the right thing - but possibly not in the right way, not
that I'm sure what the right way is :-)

Obviously the main problem was that it wasn't calculating the padding
correctly. I see you used a PRPackedBool although that looks like a waste
because there's nothing to pack it with. Also, I'm not sure about the == 1,
maybe != 0 would be better. Or change the type of mMustPad and store the
mStateData in it and test it later. Or I was also considering splitting
eRLEStateAbsoluteMode into padded and aligned states as an alternative.

The width check was originally supposed to happen in encoded mode but now I see
it doesn't work if you X delta past the width :-[

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

hm you're right, when I added mMustPad there I did think there was something to
pad with.

As for ==1 vs !=0... that doesn't really matter imho, but ok, I'll change it

I don't like storing the state data in it, because I'm not really interested in
all the data, just in the last bit...

But splitting mState sounds like a good idea... I'll make a new patch with that
done.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

now use a different value in the state enum for padded vs non-padded data

Comment 21

[[e-mail]]

Comment on attachment 123443 [details] [diff] [review]
patch v2

>                                     mStateData -= mBIH.width & 1;
>                                     if (mAlphaPtr + mStateData > mAlpha + mBIH.width)
>                                         return NS_ERROR_FAILURE;
>                                 }
>                                 memset(mAlphaPtr, 0xFF, mStateData);
>                                 mAlphaPtr += mStateData;
> 
>-                                mState = eRLEStateAbsoluteMode;
>+                                if ((mStateData % 2) == 0)
Heh, I'm a & 1 fan - see above :-)

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 123443 [details] [diff] [review]
patch v2

neil: ok, ok :)

tor: this patch fixes a crash in the bmp decoder by a) making sure that the
padding byte is correctly read/not read (this is the real fix), b) doing some
checks if we really have more data before reading it

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 123443 [details] [diff] [review]
patch v2

this patch fixes a crash in the bmp decoder by doing some data validity checks
the right way, would be good to have in 1.4.

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in nsBMPDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp,v  <-- 
nsBMPDecoder.cpp
new revision: 1.22; previous revision: 1.21
done
Checking in nsBMPDecoder.h;
/cvsroot/mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.h,v  <--  nsBMPDecoder.h
new revision: 1.17; previous revision: 1.16
done

Comment 25

[Karsten Düsterloh]

FWIW:
I can still see this with Win2000 in 2003-05-17-08 (but this may be due to
check-in after build).

I've tracked the bug down with some zip-nightlies, though:
2003-04-04 shows *all four* BMPs of bug 196977, 
but 2003-04-10 crashes on *all* of them.

Be sure to have your cache cleared before testing!

Comment 26

[Karsten Düsterloh]

With build 2003-05-19-04 I can load both 256 color graphics of 196977, but both
16 color graphics end up with an error text: "The image [URL] cannot be
displayed, because it contains errors."

So Mozilla doesn't crash anymore, but I think that it's still a *regression*
that needs investigation!

Comment 27

[Karsten Düsterloh]

Carrying over remaining regression issue to bug 206312.

Comment 28

[Matthias Versen [:Matti]]

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