21599 - mozilla crashes in MSVCRT.DLL on http://www.iconics.co.uk

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[Bruce Robson]

I am running the nightly build from 12th Dec 1999 and have my start-up page
set to a blank page. I am running under Win NT 4 SP5.

If http://www.iconics.co.uk is the first page I look at over the network
mozilla crashes after requesting the first file. The crash sometimes doesn't
happen if I have looked at other pages over the network first. Looking
at pages stored on my disk first does not stop the crash occuring.
I have downloaded the pages to my disk using Communicator. Mozilla can
look at these pages on disk without crashing.

Comment 1

[andreww]

Seems to load ok on win95 (build 1999121408)

Comment 2

[Bruce Robson]

I have now built Mozilla myself from 13th Dec 1999 sources. I have now had the
crash occur 2 times (out of 5 tries) with this version. The call stack for the
second crash was

memcpy(unsigned char * 0x00000000, unsigned char * 0x0135c56f, unsigned long
1721) line 171
nsCRT::memcpy(void * 0x00000000, void * 0x0135c56f, unsigned int 1721) line 107
+ 17 bytes
nsWriteToRawBuffer(void * 0x00000000, char * 0x0135c56f, unsigned int 0,
unsigned int 1721, unsigned int * 0x0012fb60) line 464 + 20 bytes
nsPipe::nsPipeInputStream::ReadSegments(nsPipe::nsPipeInputStream * const
0x02497948, unsigned int (void *, char *, unsigned int, unsigned int, unsigned
int *)* 0x1003e220 nsWriteToRawBuffer(void *, char *, unsigned int, unsigned
int, unsigned int *), void * 0x00000000, unsigned int 4294966984, unsigned int *
0x0012fc70) line 380 + 25 bytes
nsPipe::nsPipeInputStream::Read(nsPipe::nsPipeInputStream * const 0x02497948,
char * 0x00000000, unsigned int 4294966984, unsigned int * 0x0012fc70) line 473
nsParser::OnDataAvailable(nsParser * const 0x0131ea3c, nsIChannel * 0x024a3ba0,
nsISupports * 0x00000000, nsIInputStream * 0x02497948, unsigned int 0, unsigned
int 4294966984) line 1285 + 30 bytes
nsDocumentBindInfo::OnDataAvailable(nsDocumentBindInfo * const 0x0249c8c0,
nsIChannel * 0x024a3ba0, nsISupports * 0x00000000, nsIInputStream * 0x02497948,
unsigned int 0, unsigned int 4294966984) line 1450 + 43 bytes
nsDocumentOpenInfo::OnDataAvailable(nsDocumentOpenInfo * const 0x024a3120,
nsIChannel * 0x024a3ba0, nsISupports * 0x00000000, nsIInputStream * 0x02497948,
unsigned int 0, unsigned int 4294966984) line 216 + 46 bytes
nsChannelListener::OnDataAvailable(nsChannelListener * const 0x0249a8f0,
nsIChannel * 0x024a3ba0, nsISupports * 0x00000000, nsIInputStream * 0x02497948,
unsigned int 0, unsigned int 4294966984) line 1599
nsHTTPResponseListener::OnDataAvailable(nsHTTPResponseListener * const
0x024978d0, nsIChannel * 0x0248fd90, nsISupports * 0x024a3ba0, nsIInputStream *
0x02497948, unsigned int 0, unsigned int 4294966984) line 193 + 58 bytes
nsOnDataAvailableEvent::HandleEvent(nsOnDataAvailableEvent * const 0x024994b0)
line 370
nsStreamListenerEvent::HandlePLEvent(PLEvent * 0x02499500) line 93 + 12 bytes
PL_HandleEvent(PLEvent * 0x02499500) line 522 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00f79390) line 483 + 9 bytes
_md_EventReceiverProc(void * 0x00cb0212, unsigned int 49310, unsigned int 0,
long 16225168) line 947 + 9 bytes
DispatchMessageWorker@8 + 135 bytes
DispatchMessageA@4 + 11 bytes
nsAppShell::Run(nsAppShell * const 0x00fa14a0) line 89
nsAppShellService::Run(nsAppShellService * const 0x00f78c60) line 482
main1(int 1, char * * 0x00ef4340) line 609 + 32 bytes
main(int 1, char * * 0x00ef4340) line 677 + 13 bytes
mainCRTStartup() line 338 + 17 bytes
BaseProcessStart@4 + 64 bytes

Comment 3

[Bruce Robson]

Mozilla opens a connection to http://www.iconics.co.uk
Later mozilla detects that 15 bytes are available on an the connection.

1)  nsHTTPResponseListner::OnDataAvailable is called with i_Length == 15
2)  As this is the first data the status line has not been processed so
    nsHTTPResponseListner::ParseStatusLine is called with aLength == 15
3)  This calls Search (no length is passed) to find the first '\n' and
    therefore the length of the header. Search calls GetReadSegment to get the
    received data, by now there are 2048 bytes available that start
    "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\n" so Search finds the '\n'
    at offset 16.
4)  nsHTTPResponseListener::ParseStatusLine is happy to process more bytes than
    the number indicated by its aLength parameter. It processes all 17 bytes of
    the header and passes 17 back via *aBytesRead parameter.
5)  nsHTTPResponseListner::OnDataAvailable does not allow for this so subtracts
    17 from 15 in i_Length to get 4194967294.

ParseStatusLine allowing for more byes when OnDataAvailable does not is the
source of the problem.

The actual crash occurs due to a latter attempt to allocate almost 4GBytes
failing and the returned NULL pointer not being checked but passed to memcpy. I
have raised a new bug (number 21912) on the returned NULL not being checked.

Comment 4

[Bruce Robson]

Changed Platform to All. Also set Priority to P1 now I have a suggested patch to
be considered.

Patch as follows (n.b. diff is against 1.75 which is not the latest version)

--- nsHTTPResponseListener.org  Wed Dec 08 03:23:14 1999
+++ nsHTTPResponseListener.cpp  Fri Dec 17 12:21:45 1999
@@ -130,6 +130,7 @@
         //
         if (!mFirstLineParsed) {
             rv = ParseStatusLine(bufferInStream, i_Length, &actualBytesRead);
+            NS_ASSERTION(i_Length - actualBytesRead <= i_Length, "wrap
around");
             i_Length -= actualBytesRead;
         }

@@ -328,8 +329,10 @@
   }

   // Look for the LF which ends the Status-Line.
+  // n.b. Search looks at all pending data not just the first aLength bytes
   rv = in->Search("\n", PR_FALSE, &bFoundString, &offsetOfEnd);
   if (NS_FAILED(rv)) return rv;
+  if (bFoundString && offsetOfEnd >= aLength) bFoundString = PR_FALSE;

   if (!bFoundString) {
     //

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Changing component to Networking and reassigning, in the hopes that this patch
will be looked at.  Note that there is slight corruption of the patch due to
line-wrap.  It's better to attach patches rather than paste them in the
textarea.

Comment 6

[Andreas Buschka]

If the patch is already in CVS, it worked. No problems loading and displaying
this page with Build 1999121708 (Using Win2000 Beta3)

Comment 7

[Scott Furman]

I think the right thing to do is just completely ignore the i_Length value
passed into OnDataAvailable().  The socket transport fills the pipe on a
separate thread from the one parsing the headers, so the number of available
bytes can increase between the time an OnDataAvailable event is posted and the
time it's processed.  Furthermore, I don't see any reason why the i_Length
argument is even required.  The only place it's used is in a call to
nsIPipe::ReadSegments and one might just as well pass in (PRUint32)-1 as an
argument there.

Comment 8

[Scott Furman]

Update:

Although the i_Length argument passed to
nsHTTPResponseListener::OnDataAvailable() isn't needed by ParseStatusLine() or
ParseHTTPHeader(), it is used to compute the number of available bytes for the
mResponseDataListener.  (The mResponseDataListener is the stream listener that
consumes only the entity portion of the HTTP response, i.e. the actual content
after headers are stripped off.)  If ParseStatusLine() or ParseHTTPHeader()
consume more bytes than notified,  then we get into a situation in which
subsequent OnDataAvailable calls are received for stream data that has already
been consumed.  This requires a bit of bookkeeping to keep track of when the
OnDataAvailable notifications catch up with the actual consumption of stream
data.

Anyway, I wrote this bookkeeping code up and it came out gnarly and
twisty - complexity without much benefit.  So, I reverse my earlier opinion on
the right approach to fixing this bug.  I think that the patch that you proposed
is actually the better approach, since it's simpler.  As you've noted though, a
similar change should be made to ParseHTTPHeader().

Comment 9

[Scott Furman]

Incidentally, these comments should be coming from [e-mail] - the
[e-mail] comments are a result of a stuck cookie.

Comment 10

[Judson Valeski]

taking ownership.

Comment 11

[Judson Valeski]

Attachment: HTTPResponseLIstener full patch

Comment 12

[Judson Valeski]

I've attached a patch that makes the length check everywhere we search. Fur or
Gagan, please review.

Comment 13

[Judson Valeski]

fix checked in 12/22/99 2:16 pm pac time.

Comment 14

[leger]

Updating QA contact for verification

Comment 15

[Tom Everingham]

verified:
NT 4.0 2000011210
MAC 8.6 2000011208
LINUX 6.0 2000011208

Comment 16

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/344434ec63f1
[fenix] For https://github.com/mozilla-mobile/fenix/issues/21599 - Add UTM parameters for Pocket recommendations links