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

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Frederik Reiss]

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.

Comment 1

[Frederik Reiss]

Attachment: the html file showing this bug

Comment 2

[Frederik Reiss]

Attachment: size: 33x16

Comment 3

[Frederik Reiss]

Attachment: size: 34x16

Comment 4

[Frederik Reiss]

Attachment: size: 32x16

Comment 5

[Frederik Reiss]

Attachment: size: 33x15

Comment 6

[Frederik Reiss]

Attachment: size: 33x17

Comment 7

[Brian Polidoro]

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

Comment 8

[Brian Polidoro]

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

Comment 9

[Jackie Liu]

I can reproduce in "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060918 BonEcho/2.0".

Comment 10

[Jackie Liu]

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

Comment 11

[Jackie Liu]

Talkback IDs

In Firefox 1.5.0.7: TB23545563G
In Firefox 2.0: TB23545380

Comment 12

[Adam Guthrie]

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

Comment 13

[Martijn Wargers (dead)]

Attachment: 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.

Comment 14

[Martijn Wargers (dead)]

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.

Comment 15

[Martijn Wargers (dead)]

(In reply to comment #14)
> So, my guess is somehow a regression from bug 228662.

Ugh, I meant bug 286622.

Comment 16

[Martijn Wargers (dead)]

Yes, backing out the patch from bug 286622 seems to fix the crash.

Comment 17

[Martijn Wargers (dead)]

Attachment: this fixes the crash

Only changing this also seems to fix the crash.

Comment 18

[Daniel Veditz [:dveditz]]

Assigning to martijn to drive reviews and checkins.

Comment 19

[Martijn Wargers (dead)]

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.

Comment 20

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

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 :)

Comment 21

[Martijn Wargers (dead)]

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.

Comment 22

[Martijn Wargers (dead)]

I'm sorry, I don't know how to fix this.

Comment 23

[Mike Schroepfer]

Dbaron can you take a look or recommend a new owner?

Comment 24

[Jay Patel [:jay]]

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?

Comment 25

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 26

[Mats Palmgren (inactive)]

Attachment: Patch

The style change is needed in a debug build...

Comment 27

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

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

Comment 28

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

and, thanks for finding this!

Comment 29

[Mats Palmgren (inactive)]

(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?

Comment 30

[Daniel Veditz [:dveditz]]

Yes, please fix up CreateOpaqueAlphaChannel also sr=dveditz for the same change, I'm sure biesi would agree.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 247143 [details] [diff] [review]
Patch

approved for 1.8/1.8.0 branches, a=dveditz

Comment 32

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

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)

Comment 33

[Mats Palmgren (inactive)]

Attachment: 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.)

Comment 34

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2b

A variant of rev.2 with initialized padding... in case you prefer this.

Comment 35

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

Comment on attachment 247280 [details] [diff] [review]
Patch rev. 2

don't think we need to initialize the padding

Comment 36

[Daniel Veditz [:dveditz]]

Comment on attachment 247143 [details] [diff] [review]
Patch

Comment 32 is a minus for this patch.

Comment 37

[Daniel Veditz [:dveditz]]

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

Comment 38

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

s/the buffer/each row/?

Comment 39

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

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...

Comment 40

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

With comments and zeroing the padding.

Comment 41

[Daniel Veditz [:dveditz]]

Comment on attachment 247375 [details] [diff] [review]
Patch rev. 3

sr=dveditz

Comment 42

[Daniel Veditz [:dveditz]]

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

Comment 43

[Daniel Veditz [:dveditz]]

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.

Comment 44

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

Comment on attachment 247375 [details] [diff] [review]
Patch rev. 3

looks good now, thanks. sorry that this took so many iterations...

Comment 45

[Mats Palmgren (inactive)]

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

-> FIXED

Comment 46

[Bob Clary [:bc] (inactive)]

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