111830 - freeze/hang when retrieving malformed response for GET /favicon.ico

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[R.K.Aa.]

current linux CVS

Steps to reproduce:

-Try load http://webmail.tele2.no in browser
-freeze

No idea about component - sorry. Top shows mozilla-bin at 99% CPU continously.
Stepping through a non-debug in gdb shows an endless loop before even gdb
eventually failed.

Received Trace/breakpoint trap in LWP 18439 while waiting for SIGSTOP.
Using pending wait status for LWP 18439.

Comment 1

[R.K.Aa.]

Freezes with 2001111908, 2001112308 and current CVS.
Does NOT freeze with the 0.9.6 release.

Comment 2

[Ronald van Kuijk]

Confirmed.... In the statusbar the text says 'Resolving....' so it seems to be a
problem early in the process 

Comment 3

[Maik Amelmann]

Not a linux problem, btw. Can confirm this under win98 with 2001111708.

Comment 4

[Olivier Cahagne]

confirming with build 2001112308 on Linux even when JavaScript is disactivated.
I saved both the html and gif files (via sniffer) onto local filesystem but
can't reproduce lock when loading these files from filesystem or from a local
HTTPD daemon.

Comment 5

[Matthias Versen [:Matti]]

It works if I disable the favicon !
with favicon and win2k build 20011124.. i see a hang !

-> Imagelib (hyatt)

win2k stack trace :
nsICODecoder::ProcessData(const char * 0x058b658e, unsigned int 0) line 230 + 24
bytes
nsICODecoder::WriteFrom(nsICODecoder * const 0x04c4c0f8, nsIInputStream *
0x058a1990, unsigned int 14, unsigned int * 0x0012fb50) line 201 + 16 bytes
imgRequest::OnDataAvailable(imgRequest * const 0x0598d9d0, nsIRequest *
0x0598d8c0, nsISupports * 0x00000000, nsIInputStream * 0x058a1990, unsigned int
0, unsigned int 14) line 720 + 47 bytes
ProxyListener::OnDataAvailable(ProxyListener * const 0x0598dbd0, nsIRequest *
0x0598d8c0, nsISupports * 0x00000000, nsIInputStream * 0x058a1990, unsigned int
0, unsigned int 14) line 501
nsStreamListenerTee::OnDataAvailable(nsStreamListenerTee * const 0x04c4c020,
nsIRequest * 0x0598d8c0, nsISupports * 0x00000000, nsIInputStream * 0x0598e688,
unsigned int 0, unsigned int 14) line 56 + 51 bytes
nsHttpChannel::OnDataAvailable(nsHttpChannel * const 0x0598d8c4, nsIRequest *
0x0598e8bc, nsISupports * 0x00000000, nsIInputStream * 0x0598e688, unsigned int
0, unsigned int 14) line 2349 + 57 bytes
nsOnDataAvailableEvent::HandleEvent() line 193 + 70 bytes
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x0593b694) line 80
PL_HandleEvent(PLEvent * 0x0593b694) line 590 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00edcba0) line 520 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x000700aa, unsigned int 49392, unsigned int 0,
long 15584160) line 1071 + 9 bytes
USER32! 77e02e98()
USER32! 77e030e0()
USER32! 77e05824()
nsAppShellService::Run(nsAppShellService * const 0x00f0c668) line 303
main1(int 2, char * * 0x003526f0, nsISupports * 0x00000000) line 1304 + 32 bytes
main(int 2, char * * 0x003526f0) line 1630 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87d08()

Comment 6

[John Morrison]

complete http response to GET http://webmail.tele2.no/favicon.ico
(between '------') [actually this is the response for any 404 resource].

------
HTTP/1.1 200 OK
Server: MDNServer
Connection: close

file not found
------

Comment 7

[John Morrison]

Need to pull this one back in, IMHO. Needs a little more defence in the icon 
decoder.

Comment 8

[Warner Young]

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

Comment 9

[John Morrison]

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

Comment 10

[Matthias Versen [:Matti]]

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

Comment 11

[Jerome Lacoste]

With debug messages it should be much easier to debug. One should run the
browser with nspr debug output enabled for "imgRequest" module.

E.g. on windows, in a batch file:
set NSPR_LOG_MODULES=imgRequest:5
set NSPR_LOG_FILE=C:\Temp\mozilla_log.txt
"C:\Program Files\mozilla.org\Mozilla\mozilla.exe"

Then attach the log file on this bug. 

It outputs nothing with my builds: (I have one from 2311 and 2911). Today's
build cause data-loss, so I won't test it.

Anybody?

Comment 12

[Jerome Lacoste]

Thanks to Gilles on #mozillazine, I got a log 

[...]
1024[8060dd8]: -1756220885 [this=86dab40] imgRequest::FrameChanged {EXIT}
1024[8060dd8]: -1756220884 [this=86fbdb8] imgRequest::GetURI
1024[8060dd8]: -1756220884 [this=86fbdb8] imgRequest::GetImage
1024[8060dd8]: -1756220883 [this=86dab40] imgRequest::GetImage
1024[8060dd8]: -1756220811 [this=88818c8] imgRequest::OnStartRequest {ENTER}
1024[8060dd8]: -1756220811 [this=88818c8] imgRequest::GetURI
1024[8060dd8]: -1756220811 [this=8839388] imgRequestProxy::OnStartRequest
(name="http://webmail.tele2.no/favicon.ico")
1024[8060dd8]: -1756220811 [this=88818c8] imgRequest::OnStartRequest {EXIT}
1024[8060dd8]: -1756220809 [this=88818c8] imgRequest::OnDataAvailable
(count="14") {ENTER}
1024[8060dd8]: -1756220809 [this=88818c8] imgRequest::OnDataAvailable |First
time through... finding mimetype| {ENTER}
1024[8060dd8]: -1756220809 [this=88818c8] imgRequest::OnDataAvailable |sniffing
of mimetype failed| {ENTER}
1024[8060dd8]: -1756220809 [this=88818c8] imgRequest::OnDataAvailable -- Got
content type from the channel
1024[8060dd8]: -1756220809 [this=88818c8] imgRequest::OnDataAvailable |sniffing
of mimetype failed| {EXIT}
1024[8060dd8]: -1756220809 [this=88818c8] imgRequest::OnDataAvailable (content
type="image/x-icon")
1024[8060dd8]: -1756220808 [this=88818c8] imgRequest::SetImage
1024[8060dd8]: -1756220808 [this=88818c8] imgRequest::OnDataAvailable |First
time through... finding mimetype| {EXIT}


That should help.

Comment 13

[John Morrison]

actually bug 113579 is a slightly different case, but essentially the same.
the request for http://www.babelfish.com/favicon.ico returns 0 bytes (closes
the socket immediately). 

[This (an empty buffer) actually should never get passed to any decoder by 
imgRequest::OnDataAvailable; that could detect the zero buffer when it 
does inStr->ReadSegments to sniff the first few bytes of the content.]

Comment 14

[Matthias Versen [:Matti]]

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

Comment 15

[Matthias Versen [:Matti]]

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

Comment 16

[Matthias Versen [:Matti]]

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

Comment 17

[John Morrison]

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

Comment 18

[John Morrison]

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

Comment 19

[Matthias Versen [:Matti]]

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

Comment 20

[Julien Cegarra]

Bug 114835 dont seems to be a dupe of this one, in fact, the url 
http://webmail.registeredsite.com/en_US/cgi-bin/gx.cgi/AppLogic+moblogin freeze 
mozilla and it dont use any favicon in IE.

Comment 21

[Basil Fritts]

Julien, that's the issue -- there /isn't/ a favicon to get. See bug 110357. Your
bug is an exact dupe of that one -- even the server and cgi script look to be
the same. And that one, my bug, has been duped to this one. Check the http logs
in that bug to see why it's a dupe of this one.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

[Matthias Versen [:Matti]]

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

Comment 24

[Matthias Versen [:Matti]]

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

Comment 25

[David Caswell]

Now qualifies for MOSTFREQ, right? (12 dupes.)

 Though in this situation, I don't know who'd have the authority to put it on radar.

Comment 26

[Brendan Eich [:brendan]]

Can we get a patch for 0.9.7?  [e-mail] request it.  If this is an
iloop or a near-infinite loop, can we defend with more sanity-checking (say, of
mNumIcons in nsICODecoder::ProcessData)?

/be 

Comment 27

[Brendan Eich [:brendan]]

mNumIcons should be initialized in nsICODecoder::nsICODecoder, btw.  Cc'ing
biesi, who may be able to help quickly for 0.9.7.

/be

Comment 28

[Brendan Eich [:brendan]]

bbaetz is helping me review nsICODecoder::ProcessData, and he points out that
the while (mCurrIcon < mNumIcons) { ... } loop implicated by the stack here only
conditionally increments mCurrIcon -- that's bad.  It needs some sanity-checking
or other logic to avoid being an iloop for certain bad inputs.

/be

Comment 29

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

brendan: I had a look at this, but was unable to reproduce the hang, so fixing
it is probably difficult.

I found out, though, that the ICO Decoder doesn't check if the file is really an
.ico file... I can fix this.

Comment 30

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

copying url from dup here with which I can reproduce the hang

Comment 31

[Brendan Eich [:brendan]]

biesi: can't we at least make sure that the loop will terminate?  What is that
magic 22 buried in the condition guarding the mCurrIcon++?

/be

Comment 32

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

brendan: I can reproduce the bug now and am working on a fix

Comment 33

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

Attachment: Fix

brendan: 22 is sizeof(mDirEntryArray) + DIRENTRYOFFSET

This patch also initializes mNumIcons in the constructor as suggested in the
bug.
And it does another small optimization in the decoder.

Comment 34

[Stuart Parmenter]

Comment on attachment 61981 [details] [diff] [review]
Fix

r=pavlov

Comment 35

[David Hyatt]

sr=hyatt, use 2-space indentation rather than 4 on the return though.

Comment 36

[Brendan Eich [:brendan]]

Comment on attachment 61981 [details] [diff] [review]
Fix

a=[e-mail] for checkin to the 0.9.7 branch.

/be

Comment 37

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

timeless checked this into the trunk
branch is closed atm, so not yet checked in there

Comment 38

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

timeless also checked this into the branch.

resolving.

Comment 39

[Basil Fritts]

Marking verified. The above url and the url referenced in bug 110357 wfm in
2001121808-trunk.