Last Comment Bug 353553 - Firefox crashes on certain png images used in the css cursor property [@ ntdll.dll + 0x11e58] [@ JS_ArenaAllocate]
: Firefox crashes on certain png images used in the css cursor property [@ ntdl...
Status: VERIFIED FIXED
[sg:critical]
: crash, regression, testcase, verified1.8.0.9, verified1.8.1.1
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: 1.8 Branch
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Mats Palmgren (:mats)
: Hixie (not reading bugmail)
:
Mentors:
: 353548 353549 (view as bug list)
Depends on:
Blocks: 286622
  Show dependency treegraph
 
Reported: 2006-09-20 13:44 PDT by Frederik Reiss
Modified: 2007-01-02 18:05 PST (History)
15 users (show)
dveditz: blocking1.8.1.1+
dveditz: blocking1.8.0.9+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
the html file showing this bug (138 bytes, text/html)
2006-09-20 13:45 PDT, Frederik Reiss
no flags Details
size: 33x16 (195 bytes, image/png)
2006-09-20 13:46 PDT, Frederik Reiss
no flags Details
size: 34x16 (195 bytes, image/png)
2006-09-20 13:47 PDT, Frederik Reiss
no flags Details
size: 32x16 (192 bytes, image/png)
2006-09-20 13:47 PDT, Frederik Reiss
no flags Details
size: 33x15 (193 bytes, image/png)
2006-09-20 13:48 PDT, Frederik Reiss
no flags Details
size: 33x17 (195 bytes, image/png)
2006-09-20 13:48 PDT, Frederik Reiss
no flags Details
testcase (591 bytes, text/html)
2006-09-21 04:51 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
this fixes the crash (1.29 KB, patch)
2006-09-21 06:27 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
cbiesinger: review-
Details | Diff | Splinter Review
Trace of Data8BitTo1Bit (6.97 KB, text/plain)
2006-11-30 20:18 PST, Mats Palmgren (:mats)
no flags Details
Patch (1.96 KB, patch)
2006-11-30 20:20 PST, Mats Palmgren (:mats)
dveditz: review-
Details | Diff | Splinter Review
Patch rev. 2 (1.79 KB, patch)
2006-12-02 11:22 PST, Mats Palmgren (:mats)
cbiesinger: review+
cbiesinger: superreview+
Details | Diff | Splinter Review
Patch rev. 2b (2.69 KB, patch)
2006-12-02 11:24 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 3 (3.71 KB, patch)
2006-12-03 19:18 PST, Mats Palmgren (:mats)
cbiesinger: review+
dveditz: superreview+
dveditz: approval1.8.0.9+
dveditz: approval1.8.1.1+
Details | Diff | Splinter Review

Description Frederik Reiss 2006-09-20 13:44:43 PDT
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 2006-09-20 13:45:52 PDT
Created attachment 239403 [details]
the html file showing this bug
Comment 2 Frederik Reiss 2006-09-20 13:46:42 PDT
Created attachment 239404 [details]
size: 33x16
Comment 3 Frederik Reiss 2006-09-20 13:47:03 PDT
Created attachment 239405 [details]
size: 34x16
Comment 4 Frederik Reiss 2006-09-20 13:47:28 PDT
Created attachment 239406 [details]
size: 32x16
Comment 5 Frederik Reiss 2006-09-20 13:48:01 PDT
Created attachment 239407 [details]
size: 33x15
Comment 6 Frederik Reiss 2006-09-20 13:48:26 PDT
Created attachment 239408 [details]
size: 33x17
Comment 7 Brian Polidoro 2006-09-20 13:50:10 PDT
*** Bug 353548 has been marked as a duplicate of this bug. ***
Comment 8 Brian Polidoro 2006-09-20 13:50:31 PDT
*** Bug 353549 has been marked as a duplicate of this bug. ***
Comment 9 Jackie Liu 2006-09-20 17:22:37 PDT
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 2006-09-20 17:27:51 PDT
Moved to Core>Style System (version 1.8 branch).  Please correct if I'm incorrect.
Comment 11 Jackie Liu 2006-09-20 18:32:36 PDT
Talkback IDs

In Firefox 1.5.0.7: TB23545563G
In Firefox 2.0: TB23545380
Comment 12 Adam Guthrie 2006-09-20 20:44:30 PDT
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 [:mwargers] (not working for Mozilla) 2006-09-21 04:51:40 PDT
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.
Comment 14 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-09-21 05:44:15 PDT
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 [:mwargers] (not working for Mozilla) 2006-09-21 05:48:29 PDT
(In reply to comment #14)
> So, my guess is somehow a regression from bug 228662.

Ugh, I meant bug 286622.
Comment 16 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-09-21 06:02:26 PDT
Yes, backing out the patch from bug 286622 seems to fix the crash.
Comment 17 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-09-21 06:27:03 PDT
Created attachment 239498 [details] [diff] [review]
this fixes the crash

Only changing this also seems to fix the crash.
Comment 18 Daniel Veditz [:dveditz] 2006-11-10 16:20:47 PST
Assigning to martijn to drive reviews and checkins.
Comment 19 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-11-10 21:46:15 PST
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) 2006-11-11 08:38:07 PST
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 [:mwargers] (not working for Mozilla) 2006-11-11 08:41:06 PST
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 [:mwargers] (not working for Mozilla) 2006-11-21 15:54:01 PST
I'm sorry, I don't know how to fix this.
Comment 23 Mike Schroepfer 2006-11-22 09:33:04 PST
Dbaron can you take a look or recommend a new owner?
Comment 24 Jay Patel [:jay] 2006-11-30 13:06:49 PST
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 (:mats) 2006-11-30 20:18:14 PST
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.
Comment 26 Mats Palmgren (:mats) 2006-11-30 20:20:13 PST
Created attachment 247143 [details] [diff] [review]
Patch

The style change is needed in a debug build...
Comment 27 Christian :Biesinger (don't email me, ping me on IRC) 2006-12-01 12:53:31 PST
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) 2006-12-01 12:56:27 PST
and, thanks for finding this!
Comment 29 Mats Palmgren (:mats) 2006-12-01 14:06:52 PST
(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] 2006-12-01 18:44:18 PST
Yes, please fix up CreateOpaqueAlphaChannel also sr=dveditz for the same change, I'm sure biesi would agree.
Comment 31 Daniel Veditz [:dveditz] 2006-12-01 18:44:57 PST
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) 2006-12-02 01:29:52 PST
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 (:mats) 2006-12-02 11:22:30 PST
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.)
Comment 34 Mats Palmgren (:mats) 2006-12-02 11:24:23 PST
Created attachment 247281 [details] [diff] [review]
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) 2006-12-03 07:02:22 PST
Comment on attachment 247280 [details] [diff] [review]
Patch rev. 2

don't think we need to initialize the padding
Comment 36 Daniel Veditz [:dveditz] 2006-12-03 08:08:45 PST
Comment on attachment 247143 [details] [diff] [review]
Patch

Comment 32 is a minus for this patch.
Comment 37 Daniel Veditz [:dveditz] 2006-12-03 08:18:43 PST
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) 2006-12-03 10:48:01 PST
s/the buffer/each row/?
Comment 39 Christian :Biesinger (don't email me, ping me on IRC) 2006-12-03 10:51:41 PST
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 (:mats) 2006-12-03 19:18:25 PST
Created attachment 247375 [details] [diff] [review]
Patch rev. 3

With comments and zeroing the padding.
Comment 41 Daniel Veditz [:dveditz] 2006-12-04 10:59:08 PST
Comment on attachment 247375 [details] [diff] [review]
Patch rev. 3

sr=dveditz
Comment 42 Daniel Veditz [:dveditz] 2006-12-04 12:00:44 PST
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] 2006-12-04 14:17:36 PST
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) 2006-12-04 15:03:42 PST
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 (:mats) 2006-12-04 16:35:12 PST
Checked in to trunk at 2006-12-04 15:47 PST.

-> FIXED
Comment 46 Bob Clary [:bc:] 2006-12-06 13:37:40 PST
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

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