Last Comment Bug 245631 - Crash loading .ico file [@ nsICODecoder::ProcessData ]
: Crash loading .ico file [@ nsICODecoder::ProcessData ]
Status: VERIFIED FIXED
have patch
: crash, fixed-aviary1.0.5, fixed1.7.9, testcase
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 All
: -- critical with 5 votes (vote)
: ---
Assigned To: Son Le
:
: Milan Sreckovic [:milan]
Mentors:
: 249408 255122 290974 (view as bug list)
Depends on:
Blocks: 295457
  Show dependency treegraph
 
Reported: 2004-06-04 23:08 PDT by Peter van der Woude [:Peter6]
Modified: 2006-03-12 17:35 PST (History)
22 users (show)
dveditz: blocking1.7.9+
dveditz: blocking‑aviary1.0.3-
dveditz: blocking‑aviary1.0.5+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Stacktrace Mozilla debug build 20040604 (4.22 KB, text/plain)
2004-06-05 02:40 PDT, Olivier Cahagne
no flags Details
Reduced testcase (ICO file): warning, may crash (2.45 KB, image/x-icon)
2004-06-05 02:46 PDT, Olivier Cahagne
no flags Details
Testcase: original file reduced to 190k (crash) (191.01 KB, image/x-icon)
2004-06-05 03:49 PDT, sidki
no flags Details
TB75662X data (2.63 KB, text/plain)
2004-06-07 01:50 PDT, Olivier Cahagne
no flags Details
check memory pointers before decoding (1.86 KB, patch)
2005-03-15 05:30 PST, Son Le
dveditz: superreview-
Details | Diff | Splinter Review
patch v1 (2.12 KB, patch)
2005-03-16 00:31 PST, Son Le
cbiesinger: review-
dveditz: superreview+
Details | Diff | Splinter Review
patch v2 (3.71 KB, patch)
2005-03-21 01:25 PST, Son Le
cbiesinger: review-
Details | Diff | Splinter Review
patch v3 (3.67 KB, patch)
2005-03-22 00:57 PST, Son Le
cbiesinger: review+
Details | Diff | Splinter Review
patch v4 (3.83 KB, patch)
2005-04-06 07:32 PDT, Son Le
cbiesinger: review-
Details | Diff | Splinter Review
something like this, maybe (2.26 KB, patch)
2005-04-10 16:00 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details | Diff | Splinter Review
patch v5 (4.72 KB, patch)
2005-04-13 07:58 PDT, Son Le
cbiesinger: review+
dveditz: superreview-
Details | Diff | Splinter Review
patch v6 (5.91 KB, patch)
2005-04-20 06:15 PDT, Son Le
cbiesinger: review+
Details | Diff | Splinter Review
patch v7 (6.10 KB, patch)
2005-04-20 07:43 PDT, Son Le
cbiesinger: review+
dveditz: superreview+
dveditz: approval‑aviary1.0.5+
dveditz: approval1.7.9+
asa: approval1.8b2+
Details | Diff | Splinter Review
firefox 1.0.5 patch (5.81 KB, patch)
2005-06-11 07:19 PDT, Son Le
no flags Details | Diff | Splinter Review
branch patch (5.09 KB, patch)
2005-06-11 09:19 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details | Diff | Splinter Review

Description Peter van der Woude [:Peter6] 2004-06-04 23:08:24 PDT
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 2004-06-05 00:33:20 PDT
Worksforme with aviary 20040604 zipped build, Win2K.
Comment 2 sidki 2004-06-05 02:03:41 PDT
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 2004-06-05 02:40:48 PDT
Created attachment 150069 [details]
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 2004-06-05 02:46:51 PDT
Created attachment 150070 [details]
Reduced testcase (ICO file): warning, may crash
Comment 5 Olivier Cahagne 2004-06-05 02:51:01 PDT
CC'ing biesi.
Comment 6 sidki 2004-06-05 03:49:17 PDT
Created attachment 150071 [details]
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 2004-06-07 01:50:39 PDT
Created attachment 150185 [details]
TB75662X data
Comment 8 timeless 2004-06-07 03:41:30 PDT
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 2004-06-25 02:39:41 PDT
as a Workaround, set in about:config browser.chrome.favicons to false (this will
avoid the display of favicons in all pages...)
Comment 10 Ted Mielczarek [:ted.mielczarek] 2004-07-01 09:26:23 PDT
*** Bug 249408 has been marked as a duplicate of this bug. ***
Comment 11 Ryan Polk (Quark) 2004-08-10 19:51:48 PDT
*** Bug 255122 has been marked as a duplicate of this bug. ***
Comment 12 neil@parkwaycc.co.uk 2004-09-09 08:22:10 PDT
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) 2005-03-01 08:40:56 PST
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 2005-03-15 05:30:39 PST
Created attachment 177481 [details] [diff] [review]
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 2005-03-15 07:27:31 PST
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] 2005-03-15 12:19:51 PST
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 2005-03-16 00:31:51 PST
Created attachment 177589 [details] [diff] [review]
patch v1

new patch with extra check.
Comment 18 Daniel Veditz [:dveditz] 2005-03-16 08:45:16 PST
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) 2005-03-16 11:05:10 PST
> 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] 2005-03-16 13:33:16 PST
(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) 2005-03-19 18:35:20 PST
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 2005-03-21 01:25:23 PST
Created attachment 178100 [details] [diff] [review]
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) 2005-03-21 13:56:36 PST
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 2005-03-22 00:57:06 PST
Created attachment 178219 [details] [diff] [review]
patch v3

third time's the charm
Comment 25 Christian :Biesinger (don't email me, ping me on IRC) 2005-03-22 10:01:13 PST
Comment on attachment 178219 [details] [diff] [review]
patch v3

excellent, thanks!
Comment 26 Daniel Veditz [:dveditz] 2005-04-04 13:21:59 PDT
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] 2005-04-04 13:22:41 PDT
Not holding 1.0.3 for this crash fix
Comment 28 Son Le 2005-04-06 07:32:19 PDT
Created attachment 179845 [details] [diff] [review]
patch v4

re-added runtime null-checks
Comment 29 Christian :Biesinger (don't email me, ping me on IRC) 2005-04-10 15:49:20 PDT
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) 2005-04-10 16:00:08 PDT
Created attachment 180328 [details] [diff] [review]
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 2005-04-13 07:58:38 PDT
Created attachment 180586 [details] [diff] [review]
patch v5

latest patch - incorporating biesi's patch
Comment 32 Christian :Biesinger (don't email me, ping me on IRC) 2005-04-18 15:10:38 PDT
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] 2005-04-19 17:05:30 PDT
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 2005-04-20 06:15:43 PDT
Created attachment 181276 [details] [diff] [review]
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) 2005-04-20 06:56:09 PDT
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 2005-04-20 07:43:01 PDT
Created attachment 181286 [details] [diff] [review]
patch v7

..and with asserts.
Comment 37 Daniel Veditz [:dveditz] 2005-04-20 08:41:50 PDT
Comment on attachment 181286 [details] [diff] [review]
patch v7

sr=dveditz
Comment 38 Son Le 2005-04-27 18:13:44 PDT
Comment on attachment 181286 [details] [diff] [review]
patch v7

low risk patch for crasher
Comment 39 Asa Dotzler [:asa] 2005-04-28 13:13:38 PDT
Comment on attachment 181286 [details] [diff] [review]
patch v7

a=asa
Comment 40 Son Le 2005-04-28 17:27:07 PDT
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) 2005-04-29 05:16:40 PDT
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] 2005-06-07 13:58:00 PDT
Comment on attachment 181286 [details] [diff] [review]
patch v7

a=dveditz
Comment 43 Son Le 2005-06-11 07:19:40 PDT
Created attachment 185951 [details] [diff] [review]
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) 2005-06-11 09:19:21 PDT
Created attachment 185960 [details] [diff] [review]
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) 2005-06-11 09:24:30 PDT
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 2005-06-11 18:13:50 PDT
ahh, I see.. thanks biesi.
Comment 47 Daniel Veditz [:dveditz] 2005-06-14 12:07:01 PDT
*** Bug 290974 has been marked as a duplicate of this bug. ***
Comment 48 sairuh (rarely reading bugmail) 2005-06-17 11:00:44 PDT
verified fixed using 200506170x-1.0.5 firefox builds on linux fc3 and mac os x
10.4.1.

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