Firefox crashes on certain png images used in the css cursor property [@ ntdll.dll + 0x11e58] [@ JS_ArenaAllocate]

VERIFIED FIXED

Status

()

Core
Widget: Win32
--
critical
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Frederik Reiss, Assigned: mats)

Tracking

(5 keywords)

1.8 Branch
x86
Windows XP
crash, regression, testcase, verified1.8.0.9, verified1.8.1.1
Points:
---
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature)

Attachments

(9 attachments, 4 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7

When i use the "cursor:url(...);" css property Firefox crashes on certain images used as cursor. The crash happens only in Windows XP SP2 but not under Linux (didn't test in OSX). And it also seems to depend on the size of the image used:

crash:
  32x16.png
  33x17.png
no crash:
  32x16.png
  33x15.png
  33x17.png


Reproducible: Sometimes

Steps to Reproduce:
1. save the html file as "test.html"
2. save one of the png file as "test.png" in the same directory as the html file
3. open the html file
4. click on the "test" link sometimes
Actual Results:  
firfox crashes after some clicks

Expected Results:  
firfox should not crash

All image are plain black (no transparency) and created with The Gimp.

I tested this on some firefox versions:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060410 Firefox/1.0.8
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/2006082101 Firefox/2.0b2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/2006092004 Minefield/3.0a1

1.0.8 and 3.0a1 don't crash (and also don't display the cursor image).
1.5.0.7 an 2.0b2 display the cursor image crash.
(Reporter)

Comment 1

11 years ago
Created attachment 239403 [details]
the html file showing this bug
(Reporter)

Comment 2

11 years ago
Created attachment 239404 [details]
size: 33x16
(Reporter)

Comment 3

11 years ago
Created attachment 239405 [details]
size: 34x16
(Reporter)

Comment 4

11 years ago
Created attachment 239406 [details]
size: 32x16
(Reporter)

Comment 5

11 years ago
Created attachment 239407 [details]
size: 33x15
(Reporter)

Comment 6

11 years ago
Created attachment 239408 [details]
size: 33x17

Comment 7

11 years ago
*** Bug 353548 has been marked as a duplicate of this bug. ***

Comment 8

11 years ago
*** Bug 353549 has been marked as a duplicate of this bug. ***

Comment 9

11 years ago
I can reproduce in "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060918 BonEcho/2.0".
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox2?

Updated

11 years ago
Component: General → JavaScript Engine
Flags: blocking-firefox2?
Product: Firefox → Core
Version: unspecified → 1.8 Branch

Updated

11 years ago
Component: JavaScript Engine → Style System (CSS)

Comment 10

11 years ago
Moved to Core>Style System (version 1.8 branch).  Please correct if I'm incorrect.

Comment 11

11 years ago
Talkback IDs

In Firefox 1.5.0.7: TB23545563G
In Firefox 2.0: TB23545380
Assignee: nobody → win32
Component: Style System (CSS) → GFX: Win32
Keywords: crash
QA Contact: general → ian

Comment 12

11 years ago
Heap corruption?

Incident ID: 23545380
Stack Signature	ntdll.dll + 0x11e58 (0x7c911e58) a0651ad0
Product ID	Firefox2
Build ID	2006091813
Trigger Time	2006-09-20 18:23:41.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	ntdll.dll + (00011e58)
URL visited	
User Comments	Reproducing Bug 353549 in "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060918 BonEcho/2.0"
Since Last Crash	199 sec
Total Uptime	37273 sec
Trigger Reason	Access violation
Source File, Line No.	N/A
Stack Trace 	
ntdll.dll + 0x11e58 (0x7c911e58)
ntdll.dll + 0x18251 (0x7c918251)
ntdll.dll + 0x11c76 (0x7c911c76)
msvcrt.dll + 0x1c3c9 (0x77c2c3c9)
msvcrt.dll + 0x1c3e7 (0x77c2c3e7)
msvcrt.dll + 0x1c42e (0x77c2c42e)
JS_ArenaAllocate  [mozilla/js/src/jsarena.c, line 169]
js_AllocRawStack  [mozilla/js/src/jsinterp.c, line 339]
nsXPCWrappedJSClass::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp, line 1172]
nsXPCWrappedJS::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp, line 468]
SharedStub  [mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, line 147]
nsXMLHttpProgressEvent::GetType  [mozilla/content/base/src/nsXMLHttpRequest.h, line 216]
EventHandler  [mozilla/xpcom/proxy/src/nsProxyEvent.cpp, line 561]
SHDOCVW.dll + 0x150c24 (0x778b0c24)
nsListControlFrame::OnOptionSelected  [mozilla/layout/forms/nsListControlFrame.cpp, line 1788]
0x5bc03300
Summary: firefox crashes on certain png images used in the css cursor property → Firefox crashes on certain png images used in the css cursor property [@ ntdll.dll + 0x11e58] [@ JS_ArenaAllocate]
Created attachment 239495 [details]
testcase

With this testcase, I crash with 1.8.1 builds when hovering a couple of times over the link.

The fact that the cursor doesn't show up in trunk builds seems like a separate bug to me.
Indeed, in my debug build I get a crash because of heap corruption.
With a slightly modified testcase (using border instead of outline, because that wasn't supported for those older builds), I get a regression range between 2005-04-12 and 2005-04-14:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-04-12+05&maxdate=2005-04-14+08&cvsroot=%2Fcvsroot\
So, my guess is somehow a regression from bug 228662.
Blocks: 228662
Flags: blocking1.8.1.1?
Keywords: regression, testcase
(In reply to comment #14)
> So, my guess is somehow a regression from bug 228662.

Ugh, I meant bug 286622.
Blocks: 286622
No longer blocks: 228662
Status: NEW → ASSIGNED
Component: GFX: Win32 → Style System (CSS)
Yes, backing out the patch from bug 286622 seems to fix the crash.
Assignee: win32 → win32
Status: ASSIGNED → NEW
Component: Style System (CSS) → Widget: Win32
Created attachment 239498 [details] [diff] [review]
this fixes the crash

Only changing this also seems to fix the crash.
Assigning to martijn to drive reviews and checkins.
Assignee: win32 → martijn.martijn
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9+
Comment on attachment 239498 [details] [diff] [review]
this fixes the crash

Well, this wasn't meant as a real patch, this was just to indicate what I discovered what fixed this bug.
I don't know this code or why it fixes the bug.
Maybe this is a good patch, I don't know, else somebody has to write a decent one.
Attachment #239498 - Flags: superreview?(roc)
Attachment #239498 - Flags: review?(cbiesinger)
Comment on attachment 239498 [details] [diff] [review]
this fixes the crash

_please_ update comments when patching code. that said, why is this correct? see the comment :)
Attachment #239498 - Flags: review?(cbiesinger) → review-
Comment on attachment 239498 [details] [diff] [review]
this fixes the crash

I'm sorry, I don't understand this code. This was merely to indicate where things were going wrong.
Attachment #239498 - Flags: superreview?(roc)
I'm sorry, I don't know how to fix this.
Assignee: martijn.martijn → win32
Flags: blocking1.9?

Comment 23

11 years ago
Dbaron can you take a look or recommend a new owner?
Assignee: win32 → dbaron
Group: security
Whiteboard: [sg:critical?]
Whiteboard: [sg:critical?] → [sg:critical?] [at risk] needs patch

Comment 24

11 years ago
dbaron is away on travel and we need traction here if it's going to make
1.8.0.9/1.8.1.1.  Who else can take this bug?

Anyone in this bug willing to put together a patch?
(Assignee)

Comment 25

11 years ago
Created attachment 247142 [details]
Trace of Data8BitTo1Bit

It's a buffer overflow in nsWindow::Data8BitTo1Bit:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsWindow.cpp&rev=MOZILLA_1_8_BRANCH&root=/cvsroot&mark=2881,2896,2908,2909#2876

For example, for a 34x16 image we allocate 64 bytes (4 bytes per row)
for 'outData' - this is one byte per row to small.
(Assignee)

Comment 26

11 years ago
Created attachment 247143 [details] [diff] [review]
Patch

The style change is needed in a debug build...
Assignee: dbaron → mats.palmgren
Attachment #239498 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #247143 - Flags: superreview?(cbiesinger)
Attachment #247143 - Flags: review?(cbiesinger)
Whiteboard: [sg:critical?] [at risk] needs patch → [sg:critical?] [at risk] needs review (cbiesinger)
Comment on attachment 247143 [details] [diff] [review]
Patch

oops, thanks for catching this.

r+sr=biesi on the nsWindow.cpp change

please get dbaron or bz to review the second part, it is not obvious to me why it is correct
Attachment #247143 - Flags: superreview?(cbiesinger)
Attachment #247143 - Flags: superreview+
Attachment #247143 - Flags: review?(cbiesinger)
Attachment #247143 - Flags: review+
and, thanks for finding this!
(Assignee)

Comment 29

11 years ago
(In reply to comment #27)
> please get dbaron or bz to review the second part, it is not obvious to me
> why it is correct

Bug 303812 fixed this on trunk and it's DEBUG only so I'm skipping this part.

I searched widget/ for similar allocation expressions and I've found
one more that looks suspicious:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/widget/src/windows/nsWindow.cpp&rev=3.676&root=/cvsroot&mark=2761#2757

We don't write past the end but maybe DataToBitmap() will read
past the end?  It seems to me we should have the same change here.
What do you think?
Yes, please fix up CreateOpaqueAlphaChannel also sr=dveditz for the same change, I'm sure biesi would agree.
Whiteboard: [sg:critical?] [at risk] needs review (cbiesinger) → [sg:critical] needs approval
Comment on attachment 247143 [details] [diff] [review]
Patch

approved for 1.8/1.8.0 branches, a=dveditz
Attachment #247143 - Flags: approval1.8.1.1+
Attachment #247143 - Flags: approval1.8.0.9+
Whiteboard: [sg:critical] needs approval → [sg:critical] needs check-in
Actually... the new code is wrong too... the old code ensured that outBpr was a multiple of 4. That is still needed, because that's what HBITMAPs need.

(but yeah, I do agree that CreateOpaqueAlphaChannel needs this too)
(Assignee)

Comment 33

11 years ago
Created attachment 247280 [details] [diff] [review]
Patch rev. 2

This patch restores the padding requirement.

So, the number of bytes we need for the copying is:
(aWidth + 7) / 8
and to pad that to the next 4-byte boundary:
(((aWidth + 7) / 8) + 3) & ~3
which is equivalent to:
((aWidth + 31) / 8) & ~3

Do we have any requirements on the value of the pad bytes?
I guess CreateDIBitmap shouldn't use them, but I don't know.
(Data8BitTo1Bit leaves them uninitialized with this patch, as before.)
Attachment #247143 - Attachment is obsolete: true
Attachment #247280 - Flags: superreview?(cbiesinger)
Attachment #247280 - Flags: review?(cbiesinger)
(Assignee)

Comment 34

11 years ago
Created attachment 247281 [details] [diff] [review]
Patch rev. 2b

A variant of rev.2 with initialized padding... in case you prefer this.
Comment on attachment 247280 [details] [diff] [review]
Patch rev. 2

don't think we need to initialize the padding
Attachment #247280 - Flags: superreview?(cbiesinger)
Attachment #247280 - Flags: superreview+
Attachment #247280 - Flags: review?(cbiesinger)
Attachment #247280 - Flags: review+
Comment on attachment 247143 [details] [diff] [review]
Patch

Comment 32 is a minus for this patch.
Attachment #247143 - Flags: superreview+
Attachment #247143 - Flags: review-
Attachment #247143 - Flags: review+
Attachment #247143 - Flags: approval1.8.1.1+
Attachment #247143 - Flags: approval1.8.0.9+
Comment on attachment 247280 [details] [diff] [review]
Patch rev. 2

approved for 1.8/1.8.0 branches, a=dveditz

Please add a comment explaining the cryptic math, especially the bit about the non-obvious HBITMAP requirement.

"HBITMAPs require the buffer to be padded to a multiple of 4 bytes" is probably good enough, unless you also want to include the intermediate calculation "(((aWidth + 7) / 8) + 3) & ~3" in the comment
Attachment #247280 - Flags: approval1.8.1.1+
Attachment #247280 - Flags: approval1.8.0.9+
s/the buffer/each row/?
btw, in case you want a reference for that requirement:
http://msdn2.microsoft.com/en-us/library/ms532284.aspx
"The bits in the array are packed together, but each scan line must be padded with zeroes to end on a LONG data-type boundary."

and now that I've looked this up, it sounds like the padding does need to be initialized, doesn't it...
(Assignee)

Comment 40

11 years ago
Created attachment 247375 [details] [diff] [review]
Patch rev. 3

With comments and zeroing the padding.
Attachment #247280 - Attachment is obsolete: true
Attachment #247281 - Attachment is obsolete: true
Attachment #247375 - Flags: superreview?(cbiesinger)
Attachment #247375 - Flags: review?(cbiesinger)
Comment on attachment 247375 [details] [diff] [review]
Patch rev. 3

sr=dveditz
Attachment #247375 - Flags: superreview?(cbiesinger)
Attachment #247375 - Flags: superreview+
Attachment #247375 - Flags: approval1.8.1.1?
Attachment #247375 - Flags: approval1.8.0.9?
Comment on attachment 247375 [details] [diff] [review]
Patch rev. 3

This addresses biesi's review comments, approved for 1.8/1.8.0 branches. a=dveditz for drivers
Attachment #247375 - Flags: approval1.8.1.1?
Attachment #247375 - Flags: approval1.8.1.1+
Attachment #247375 - Flags: approval1.8.0.9?
Attachment #247375 - Flags: approval1.8.0.9+
Attachment #247280 - Flags: approval1.8.1.1+
Attachment #247280 - Flags: approval1.8.0.9+
Checked in attachment 247375 [details] [diff] [review] to the 1.8 and 1.8.0 branches. Still need a trunk check-in, and we're presuming biesi will OK it so there may need to be some branch adjustment too.
Keywords: fixed1.8.0.9, fixed1.8.1.1
Comment on attachment 247375 [details] [diff] [review]
Patch rev. 3

looks good now, thanks. sorry that this took so many iterations...
Attachment #247375 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 45

11 years ago
Checked in to trunk at 2006-12-04 15:47 PST.

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: blocking1.9?
Resolution: --- → FIXED
Whiteboard: [sg:critical] needs check-in → [sg:critical]

Comment 46

11 years ago
verified fixed 1.8.0.9, 1.8.1.1, 1.9 windows
no crash on 1.8.0.9, 1.8.1.1, 1.9 linux/macppc
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1
Group: security
Crash Signature: [@ ntdll.dll + 0x11e58] [@ JS_ArenaAllocate]
You need to log in before you can comment on or make changes to this bug.