245631 - Crash loading .ico file [@ nsICODecoder::ProcessData ]

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Peter van der Woude [:Peter6]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040602 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040602 Firefox/0.8.0+

Open http://www.renaudbray.com/ and crash

Reproducible: Always
Steps to Reproduce:




That page is bad....
but displays w/o problems in:
IE6
K-Meleon 0.8.2
Opera 7.23 and 7.50

Comment 1

[Dimitrios]

Worksforme with aviary 20040604 zipped build, Win2K.

Comment 2

[sidki]

This bug seems to be around for a while. Confirming with:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040511
Firefox/0.8.0+

It's their huge favicon that crashes Firefox for me:
http://www.renaud-bray.com/favicon.ico

HTTP/1.1 200 OK
Server: Microsoft-IIS/5.0
X-Powered-By: ASP.NET
Date: Sat, 05 Jun 2004 08:39:15 GMT
Content-Type: image/x-icon
Accept-Ranges: bytes
Last-Modified: Thu, 22 May 1997 20:13:24 GMT
Content-Length: 2439296

The sorce looks like anything else but an icon:
MSSQL   SS00153Customers 971421613...

Comment 3

[Olivier Cahagne]

Attachment: Stacktrace Mozilla debug build 20040604

if it helps, this is what I could get with my broken GDB:

(gdb) frame 1
#1  0x41e60217 in nsICODecoder::ProcessData(char const*, unsigned)
(this=Variable "this" is not available.
)
    at nsICODecoder.cpp:386
386		    memcpy(mRow + mRowBytes, aBuffer, toCopy);
Current language:  auto; currently c++
(gdb) p aBuffer
$1 = 0x86e4919 "\001"

Comment 4

[Olivier Cahagne]

Attachment: Reduced testcase (ICO file): warning, may crash

Comment 5

[Olivier Cahagne]

CC'ing biesi.

Comment 6

[sidki]

Attachment: Testcase: original file reduced to 190k (crash)

The previous reduced testcase worksforme. It starts crashing when i reduce the
original file to ~190k and it's content dependend. Hopefully i'm not spoiling
bandwidth by attaching this.

Comment 7

[Olivier Cahagne]

Attachment: TB75662X data

Comment 8

[timeless]

Comment on attachment 150185 [details]
TB75662X data

i don't think this is why we actually crash, but i'd have wanted dreftool to
complain like this (not sure why it didn't):
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/libpr0n/decoders/bm
p/nsICODecoder.cpp&rev=1.31&cvsroot=/cvsroot&mark=471-472,486,497,386,473,500,1
18,

Comment 9

[Acid Lemon]

as a Workaround, set in about:config browser.chrome.favicons to false (this will
avoid the display of favicons in all pages...)

Comment 10

[(not currently active) Ted Mielczarek]

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

Comment 11

[Ryan Polk (Quark)]

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

Comment 12

[[e-mail]]

This crash has two causes.

The first is that the first four bytes of the icon file are not validated.

The second is that the invalid image offset is before the end of the headers.

Thus we bypass the memory allocation and jump straight into decoding :-(

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

I think we want to get this investigated and fixed if necessary sooner rather
than later.  Nominating for blocking, and adding a few others who may know
what's up.

Comment 14

[Son Le]

Attachment: check memory pointers before decoding

Hmmm... very bad. Memory being allocated in different if() block to the one
using it. Proposed patch to at least check pointer validity before using.

Comment 15

[jtk]

Please also note that a favicon.ico of unlimted size will be rendered, resulting
in resource (link, disk, memory) consumption.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 177481 [details] [diff] [review]
check memory pointers before decoding

>@@ -490,16 +495,20 @@ nsresult nsICODecoder::ProcessData(const
>       delete []mRow;
>       mRow = new PRUint8[rowSize];
 +	if (!mRow)
 +	  return NS_ERROR_OUT_OF_MEMORY;
>       mAlphaBuffer = new PRUint8[mDirEntry.mHeight*rowSize];
 +	if (!mAlphaBuffer)
 +	  return NS_ERROR_OUT_OF_MEMORY;
>       memset(mAlphaBuffer, 0xff, mDirEntry.mHeight*rowSize);
>     }

We've already got problems here with that memset: these allocations should be
checked like all the others.

Once you do that, do you need the other checks? Maybe, if ProcessData() will
still be called back even after returning an error to the input stream. If
returning NS_ERROR_OUT_OF_MEMORY stops the ProcessData calls then you should
never be able to get into those conditions (once you add the above), but it
shouldn't hurt much to do the extra checks just in case.

Please incorporate these into a new patch. sr-

Comment 17

[Son Le]

Attachment: patch v1

new patch with extra check.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 177589 [details] [diff] [review]
patch v1

sr=dveditz
You still need a review from an image peer, I can't give both.

Comment 19

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

> if ProcessData() will
> still be called back even after returning an error to the input stream.

I think it is (I suck); ReadSegments throws away the return code from the
ReadSegCb function, so the caller of WriteFrom doesn't know that ProcessData
failed :-/ (but that should be fixed, not worked around)

Comment 20

[Daniel Veditz [:dveditz]]

(In reply to comment #15)
> Please also note that a favicon.ico of unlimted size will be rendered, resulting
> in resource (link, disk, memory) consumption.

That's a general problem covered in several other bugs, probably one per image
type though they're really all the same: we'll try to render it until the OS
says we can't.

Comment 21

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

Comment on attachment 177589 [details] [diff] [review]
patch v1

seems to me like a better fix would be to ensure mImageOffset is >= the size of
the headers.

(the OOM checks for mRow and mAlphaBuffer look good, though.)

Comment 22

[Son Le]

Attachment: patch v2

Alrighty. Converted checks into asserts and added logic to ensure mImageOffset
is >= the size of the headers. I think I got the logic right, but please double
check. 

Also added addition check at the beginning to make sure the file's valid.
Tested both changes independently on my Windows box.

Comment 23

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

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

+      mIsIcon = (*aBuffer == 1);

I don't think this needs to be a member variable (you can just return here if
the buffer is neither 1 nor 2). but if you do want it to be one, why not make
an enum FileType {
  eCursor,
  eIcon
} mType; ?

looks great otherwise, thanks!

Comment 24

[Son Le]

Attachment: patch v3

third time's the charm

Comment 25

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

Comment on attachment 178219 [details] [diff] [review]
patch v3

excellent, thanks!

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 178219 [details] [diff] [review]
patch v3

>@@ -391,16 +401,19 @@ nsresult nsICODecoder::ProcessData(const
> 
>+    NS_ASSERTION(mRow, "mRow is null");
>+    NS_ASSERTION(mDecodedBuffer, "mDecodedBuffer is null");

>@@ -486,20 +499,27 @@ nsresult nsICODecoder::ProcessData(const
> 
>+    NS_ASSERTION(mRow, "mRow is null");
>+    NS_ASSERTION(mAlphaBuffer, "mAlphaBuffer is null");


Based on biesi's comment 19 shouldn't we keep the runtime null-checks instead
of assertions? I don't see a patch for the problem in ReadSegments anywhere,
better safe than sorry here.

Comment 27

[Daniel Veditz [:dveditz]]

Not holding 1.0.3 for this crash fix

Comment 28

[Son Le]

Attachment: patch v4

re-added runtime null-checks

Comment 29

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

Comment on attachment 179845 [details] [diff] [review]
patch v4

I'd rather see a real solution

Comment 30

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

Attachment: something like this, maybe

this should work, but I didn't test it. has the downside that if ReadSegments
itself fails, that's not handled...

Comment 31

[Son Le]

Attachment: patch v5

latest patch - incorporating biesi's patch

Comment 32

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

Comment on attachment 180586 [details] [diff] [review]
patch v5

why not leave the assertions in? assertions are good :-)

and, sorry about the next comment since it was my patch that had this issue
too, I think it'd be better to check the ReadSegments return value in
WriteFrom. for example:
  nsresult rv = aInStr->ReadSegments(ReadSegCb, this, aCount, aRetval);
  if (NS_FAILED(rv))
    return rv;
  return mStatus;

r=me with that, and sorry for the delay :/

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 180586 [details] [diff] [review]
patch v5

sr- because I'd like to see a patch that incorporates biesi's comments -- most
likely someone else will be checking this in and will need a complete patch.

I'm still concerned about the missing null checks -- you're depending on a
whole lot of assumptions in other people's code to hold true. I'd like to see
the assertions remain at least.

Comment 34

[Son Le]

Attachment: patch v6

biesi's changes, null checks, and fixed indentation in ReadSegCb while I'm at
it

Comment 35

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

Comment on attachment 181276 [details] [diff] [review]
patch v6

I guess this is ok. but since the null checks should be impossible to hit,
there should be at least a warning, if not an assertion there...

Comment 36

[Son Le]

Attachment: patch v7

..and with asserts.

Comment 37

[Daniel Veditz [:dveditz]]

Comment on attachment 181286 [details] [diff] [review]
patch v7

sr=dveditz

Comment 38

[Son Le]

Comment on attachment 181286 [details] [diff] [review]
patch v7

low risk patch for crasher

Comment 39

[Asa Dotzler [:asa]]

Comment on attachment 181286 [details] [diff] [review]
patch v7

a=asa

Comment 40

[Son Le]

hi biesi, can I get you to do the honour of checking it in?

Comment 41

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

yep, done:

Checking in modules/libpr0n/decoders/bmp/nsICODecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/bmp/nsICODecoder.cpp,v  <-- 
nsICODecoder.cpp
new revision: 1.34; previous revision: 1.33
done
Checking in modules/libpr0n/decoders/bmp/nsICODecoder.h;
/cvsroot/mozilla/modules/libpr0n/decoders/bmp/nsICODecoder.h,v  <--  nsICODecoder.h
new revision: 1.13; previous revision: 1.12
done

Comment 42

[Daniel Veditz [:dveditz]]

Comment on attachment 181286 [details] [diff] [review]
patch v7

a=dveditz

Comment 43

[Son Le]

Attachment: firefox 1.0.5 patch

this patch should work for firefox 1.0.5

Comment 44

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

Attachment: branch patch

sorry... that patch did not apply to the 1.7 branch (and, I presume, neither to
the aviary 1.0.1 branch). this one does.

Comment 45

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

MOZILLA_1_7_BRANCH:

Checking in nsICODecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/bmp/nsICODecoder.cpp,v  <-- 
nsICODecoder.cpp
new revision: 1.30.2.2; previous revision: 1.30.2.1
done
Checking in nsICODecoder.h;
/cvsroot/mozilla/modules/libpr0n/decoders/bmp/nsICODecoder.h,v  <--  nsICODecoder.h
new revision: 1.10.2.1; previous revision: 1.10
done

AVIARY_1_0_1_20050124_BRANCH:

Checking in modules/libpr0n/decoders/bmp/nsICODecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/bmp/nsICODecoder.cpp,v  <-- 
nsICODecoder.cpp
new revision: 1.30.16.2; previous revision: 1.30.16.1
done
Checking in modules/libpr0n/decoders/bmp/nsICODecoder.h;
/cvsroot/mozilla/modules/libpr0n/decoders/bmp/nsICODecoder.h,v  <--  nsICODecoder.h
new revision: 1.10.16.1; previous revision: 1.10
done

Comment 46

[Son Le]

ahh, I see.. thanks biesi.

Comment 47

[Daniel Veditz [:dveditz]]

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

Comment 48

[sairuh (rarely reading bugmail)]

verified fixed using 200506170x-1.0.5 firefox builds on linux fc3 and mac os x
10.4.1.