The default bug view has changed. See this FAQ.

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

VERIFIED FIXED

Status

()

Core
ImageLib
--
critical
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: Peter6, Assigned: Son Le)

Tracking

(4 keywords)

Trunk
x86
All
crash, fixed-aviary1.0.5, fixed1.7.9, testcase
Points:
---
Bug Flags:
blocking1.7.9 +
blocking-aviary1.0.3 -
blocking-aviary1.0.5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: have patch, crash signature)

Attachments

(6 attachments, 9 obsolete attachments)

4.22 KB, text/plain
Details
191.01 KB, image/x-icon
Details
2.63 KB, text/plain
Details
2.26 KB, patch
Details | Diff | Splinter Review
6.10 KB, patch
Biesinger
: review+
dveditz
: superreview+
dveditz
: approval1.7.9+
Details | Diff | Splinter Review
5.09 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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
(Reporter)

Updated

13 years ago
Flags: blocking1.0?

Comment 1

13 years ago
Worksforme with aviary 20040604 zipped build, Win2K.

Comment 2

13 years ago
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...

Updated

13 years ago
Severity: major → critical
Component: General → ImageLib
Flags: blocking1.0?
Keywords: crash, talkbackid
OS: Windows 2000 → All
Product: Firefox → Browser
Summary: Firefox crashes on http://www.renaudbray.com/ → Crash loading 2.5MB binary .ico file [@ nsICODecoder::ProcessData ]
Whiteboard: TB75662X
Version: unspecified → Trunk

Comment 3

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

13 years ago
Created attachment 150070 [details]
Reduced testcase (ICO file): warning, may crash

Updated

13 years ago
Attachment #150070 - Attachment is obsolete: true

Comment 5

13 years ago
CC'ing biesi.
Assignee: firefox → jdunn
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: firefox.general

Comment 6

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

13 years ago
Created attachment 150185 [details]
TB75662X data

Updated

13 years ago
Keywords: talkbackid → clean-report
Whiteboard: TB75662X

Comment 8

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

13 years ago
as a Workaround, set in about:config browser.chrome.favicons to false (this will
avoid the display of favicons in all pages...)
*** Bug 249408 has been marked as a duplicate of this bug. ***

Updated

13 years ago
Keywords: testcase

Comment 11

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

Comment 12

13 years ago
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 :-(
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.
Flags: blocking1.8b2?
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.0.2?
(Assignee)

Comment 14

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #177481 - Flags: review?(tor)

Comment 15

12 years ago
Please also note that a favicon.ico of unlimted size will be rendered, resulting
in resource (link, disk, memory) consumption.
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-
Attachment #177481 - Attachment is obsolete: true
Attachment #177481 - Flags: superreview-
(Assignee)

Comment 17

12 years ago
Created attachment 177589 [details] [diff] [review]
patch v1

new patch with extra check.
Attachment #177589 - Flags: superreview?(dveditz)
Attachment #177589 - Flags: review?(dveditz)
(Assignee)

Updated

12 years ago
Attachment #177481 - Flags: review?(tor)
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.
Attachment #177589 - Flags: superreview?(dveditz)
Attachment #177589 - Flags: superreview+
Attachment #177589 - Flags: review?(tor)
Attachment #177589 - Flags: review?(dveditz)
> 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)
Keywords: clean-report
(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.

Updated

12 years ago
Attachment #177589 - Flags: review?(tor) → review?(cbiesinger)
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.)
Attachment #177589 - Flags: review?(cbiesinger) → review-
(Assignee)

Comment 22

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #177589 - Attachment is obsolete: true
Attachment #178100 - Flags: review?(cbiesinger)
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!
Attachment #178100 - Flags: review?(cbiesinger) → review-
(Assignee)

Comment 24

12 years ago
Created attachment 178219 [details] [diff] [review]
patch v3

third time's the charm
Attachment #178100 - Attachment is obsolete: true
Attachment #178219 - Flags: superreview?(dveditz)
Attachment #178219 - Flags: review?(cbiesinger)
Comment on attachment 178219 [details] [diff] [review]
patch v3

excellent, thanks!
Attachment #178219 - Flags: review?(cbiesinger) → review+
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.
Not holding 1.0.3 for this crash fix
Flags: blocking-aviary1.0.3? → blocking-aviary1.0.3-
(Assignee)

Comment 28

12 years ago
Created attachment 179845 [details] [diff] [review]
patch v4

re-added runtime null-checks
Attachment #178219 - Attachment is obsolete: true
Attachment #179845 - Flags: superreview?(dveditz)
Attachment #179845 - Flags: review?(cbiesinger)
(Assignee)

Updated

12 years ago
Attachment #178219 - Flags: superreview?(dveditz)
Comment on attachment 179845 [details] [diff] [review]
patch v4

I'd rather see a real solution
Attachment #179845 - Flags: review?(cbiesinger) → review-
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...
(Assignee)

Comment 31

12 years ago
Created attachment 180586 [details] [diff] [review]
patch v5

latest patch - incorporating biesi's patch
(Assignee)

Updated

12 years ago
Attachment #179845 - Attachment is obsolete: true
Attachment #180586 - Flags: superreview?(dveditz)
Attachment #180586 - Flags: review?(cbiesinger)
(Assignee)

Updated

12 years ago
Attachment #179845 - Flags: superreview?(dveditz)
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 :/
Attachment #180586 - Flags: review?(cbiesinger) → review+
Assignee: jdunn → son.le0
Summary: Crash loading 2.5MB binary .ico file [@ nsICODecoder::ProcessData ] → Crash loading .ico file [@ nsICODecoder::ProcessData ]
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.
Attachment #180586 - Flags: superreview?(dveditz) → superreview-
(Assignee)

Comment 34

12 years ago
Created attachment 181276 [details] [diff] [review]
patch v6

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

Updated

12 years ago
Attachment #180586 - Attachment is obsolete: true
Attachment #181276 - Flags: superreview?(dveditz)
Attachment #181276 - Flags: review?(cbiesinger)
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...
Attachment #181276 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 36

12 years ago
Created attachment 181286 [details] [diff] [review]
patch v7

..and with asserts.
Attachment #181276 - Attachment is obsolete: true
Attachment #181286 - Flags: superreview?(dveditz)
Attachment #181286 - Flags: review?(cbiesinger)
(Assignee)

Updated

12 years ago
Attachment #181276 - Flags: superreview?(dveditz)
Comment on attachment 181286 [details] [diff] [review]
patch v7

sr=dveditz
Attachment #181286 - Flags: superreview?(dveditz) → superreview+
Attachment #181286 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 38

12 years ago
Comment on attachment 181286 [details] [diff] [review]
patch v7

low risk patch for crasher
Attachment #181286 - Flags: approval1.8b2?

Comment 39

12 years ago
Comment on attachment 181286 [details] [diff] [review]
patch v7

a=asa
Attachment #181286 - Flags: approval1.8b2? → approval1.8b2+
(Assignee)

Comment 40

12 years ago
hi biesi, can I get you to do the honour of checking it in?
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
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Flags: blocking1.8b2?
Flags: blocking-aviary1.1?
Flags: blocking1.7.9?
Flags: blocking-aviary1.0.5?
Blocks: 295457
Attachment #181286 - Flags: approval1.7.9?
Attachment #181286 - Flags: approval-aviary1.0.5?
Flags: blocking1.7.9?
Flags: blocking1.7.9+
Flags: blocking-aviary1.0.5?
Flags: blocking-aviary1.0.5+
Whiteboard: have patch
Comment on attachment 181286 [details] [diff] [review]
patch v7

a=dveditz
Attachment #181286 - Flags: approval1.7.9?
Attachment #181286 - Flags: approval1.7.9+
Attachment #181286 - Flags: approval-aviary1.0.5?
Attachment #181286 - Flags: approval-aviary1.0.5+
(Assignee)

Comment 43

12 years ago
Created attachment 185951 [details] [diff] [review]
firefox 1.0.5 patch

this patch should work for firefox 1.0.5
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.
Attachment #185951 - Attachment is obsolete: true
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
Keywords: fixed-aviary1.0.5, fixed1.7.9
(Assignee)

Comment 46

12 years ago
ahh, I see.. thanks biesi.
*** Bug 290974 has been marked as a duplicate of this bug. ***
verified fixed using 200506170x-1.0.5 firefox builds on linux fc3 and mac os x
10.4.1.
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsICODecoder::ProcessData ]
You need to log in before you can comment on or make changes to this bug.