82200 - crashes [@ PL_strnchr] called from ParseChunkRemaining

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

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

One of the top crashes listed in
ftp://ftp.mozilla.org/pub/data/crash-data/detailed-crash-analysis-all.html
is a crash with the following stack (lines are from 5-18 build): 

PL_strnchr [../../../../lib/libc/src/strchr.c line 61] 
 nsHttpChunkedDecoder::ParseChunkRemaining
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpChunkedDecoder.cpp
line 84] 
 nsHttpChunkedDecoder::HandleChunkedContent
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpChunkedDecoder.cpp
line 55] 
 nsHttpTransaction::HandleContent
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpTransaction.cpp
line 377] 
 nsHttpTransaction::Read
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpTransaction.cpp
line 589] 
 nsReadFromInputStream [d:\builds\seamonkey\mozilla\xpcom\io\nsPipe2.cpp line
831] 
 nsPipe::nsPipeOutputStream::WriteSegments
[d:\builds\seamonkey\mozilla\xpcom\io\nsPipe2.cpp line 705] 
 nsPipe::nsPipeOutputStream::WriteFrom
[d:\builds\seamonkey\mozilla\xpcom\io\nsPipe2.cpp line 839] 
 nsReadFromInputStream [d:\builds\seamonkey\mozilla\xpcom\io\nsPipe2.cpp line
828] 
 nsHttpTransaction::OnDataReadable
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpTransaction.cpp
line 177] 
 nsHttpConnection::OnDataAvailable
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpConnection.cpp line
604] 

I'm not quite sure what could be causing this but it looks like the kind of
thing that could be solved by thinking through the code and finding what could
go wrong.  Although it may be something that's fixed already, it was still
showing up in builds from Thursday and Friday (there's generally much less
talkback data generated on the weekend, and the current report probably wouldn't
have Monday's builds yet).

Comment 1

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

FWIW, the user comments are:

Comment: crash going to bonsai 
Comment: Clicked on "New Checkins"
URL: www.tagstrance.com 
URL: crash when dowloading a file with ftp /
  Comment: crash when loading "today's checkins" on mozilla.org
URL: mousing over File->Attachments-> in mail 
URL: groups.google.com/advanced_group_search /
  Comment: Looking at articles with search:Associated statement is not prepared 
URL: www.mozilla.org  /  Comment: "New checkins" link. 
Comment: crash opening a bugzilla link in a new window
URL: mozilla.org  /  Comment: Clicked on "New Checkins"
URL: www.mozilla.org  /  Comment: clicked on new checkins
URL: weather.yahoo.com 

Comment 2

[Darin Fisher]

-> me

Comment 3

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

Reports of this crash continued in builds from Sunday (20) and Monday (21), so
it seems like it's still around.

Comment 4

[Kathleen :Brade]

running purify on a windows build from this morning, I saw an ABR at this 
location.  I had two browser windows.  I had gone to www.cnn.com and had gone to 
www.apple.com and clicked in each window.

Comment 5

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

brade:  Where exactly did the ABR occur?  Was it in the call to PL_strnchr on
line 61?

Comment 6

[Kathleen :Brade]

[E] ABR: Array bounds read in PL_strnchr {8 occurrences}
    Reading 1 byte from 0x043b8ce8 (1 byte at 0x043b8ce8 illegal)
    Address 0x043b8ce8 is 1 byte past the end of a 4096 byte block at 0x043b7ce8
    Address 0x043b8ce8 points to a malloc'd block in heap 0x02d90000
    Thread ID: 0xb3
    Error location
        PL_strnchr     [strchr.c:57]
        nsHttpChunkedDecoder::ParseChunkRemaining(char *,UINT,UINT *) 
[nsHttpChunkedDecoder.cpp:83]
        nsHttpChunkedDecoder::HandleChunkedContent(char *,UINT,UINT *) 
[nsHttpChunkedDecoder.cpp:54]
        nsHttpTransaction::HandleContent(char *,UINT,UINT *) 
[nsHttpTransaction.cpp:476]
        nsHttpTransaction::Read(char *,UINT,UINT *) [nsHttpTransaction.cpp:709]
        nsReadFromInputStream [nsPipe2.cpp:830]
        nsPipe::nsPipeOutputStream::WriteSegments((*)(nsIOutputStream *,void 
*,char *,UINT,UINT,UINT *),void *,UINT,UINT *) [nsPipe2.cpp:704]
        nsPipe::nsPipeOutputStream::WriteFrom(nsIInputStream *,UINT,UINT *) 
[nsPipe2.cpp:838]
        nsStreamListenerProxy::OnDataAvailable(nsIRequest *,nsISupports 
*,nsIInputStream *,UINT,UINT) [nsStreamListenerProxy.cpp:283]
        nsHttpTransaction::OnDataReadable(nsIInputStream *) 
[nsHttpTransaction.cpp:214]

Comment 7

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

If we're only reading one character off the end of the buffer, could the problem
be that we're using PL_strnchr on a buffer that's not null-terminated (i.e., we
don't own the character that's one past the end), and PL_strnchr isn't designed
to handle that since it checks that *s is non-null before checking that n is
nonzero:

PL_strnchr(const char *s, char c, PRUint32 n)
{
    if( (const char *)0 == s ) return (char *)0;

    for( ; *s && n; s++, n-- )
        if( *s == c )
            return (char *)s;

    if( ((char)0 == c) && ((char)0 == *s) && (n > 0)) return (char *)s;

    return (char *)0;
}

Maybe we need to be using one fewer bytes of the buffer?  Or should this be
considered a bug in NSPR?  Or do we need something like memchr?

Comment 8

[Shiva Thirumazhusai]

Is this because nsHttpChunkedDecoder is ending with PRPackedBool ? 
I saw the same problem with another class ending with PRPackedBool.

CCing waterson.

Comment 9

[Wan-Teh Chang]

If a buffer is not null-terminated, by definition it is not
a C string, therefore you can't pass it to PL_strnchr.

The standard C library function memchr, declared in <string.h>,
is exactly what you need.

Comment 10

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

I guess memchr is available on all platforms since we have code that uses it
(nsCharTraits.h, bufferRoutines,h, prscanf.c), so I'll attach a patch shortly
to make that change.  (Otherwise the autoconf-generated comment that "# SunOS
4.x string.h does not declare mem*, contrary to ANSI." would scares me.)

So I'm about to test the following patch:

Index: nsHttpChunkedDecoder.cpp
===================================================================
RCS file: /cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChunkedDecoder.cpp,v
retrieving revision 1.3
diff -u -d -r1.3 nsHttpChunkedDecoder.cpp
--- nsHttpChunkedDecoder.cpp	2001/05/14 10:15:06	1.3
+++ nsHttpChunkedDecoder.cpp	2001/06/04 23:39:50
@@ -80,7 +80,7 @@
 
     *bytesConsumed = 0;
     
-    char *p = PL_strnchr(buf, '\n', count);
+    char *p = NS_STATIC_CAST(char*, memchr(buf, '\n', count));
     if (p) {
         *p = 0;
         *bytesConsumed = p - buf + 1;

Comment 11

[Wan-Teh Chang]

Yes, every respectable C implementation has memchr().  It is
in the ANSI/ISO C standard, dated 1989.

SunOS 4.x headers fail to *declare* many standard library
functions but the functions are implemented.

Comment 12

[Darin Fisher]

what is the point of PL_strnchr if the first parameter must be null-terminated?
i'm happy to switch to memchr, but it seems really odd that PL_strnchr would
behave this way, since clearly other functions like PL_strncmp don't.

looks like nsHttpTransaction.cpp:322 will also need to be fixed.  attaching a
patch which gets both of these.

Comment 13

[Darin Fisher]

Attachment: fixes the problem

Comment 14

[Darin Fisher]

we should probably aim to get this in for 0.9.1

Comment 15

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

r=dbaron

Agreed on 0.9.1, although that requires working fast.

Comment 16

[Gagan]

r=gagan and check with chofmann on getting this in for 0.9.1 (small fix, low 
risk, high gain)

Comment 17

[Christopher Blizzard (:blizzard)]

a=blizzard for 0.9.1 for drivers since no one actually put that in the bug.

Comment 18

[Darin Fisher]

fix checked in on the trunk and the 0.9.1 branch.

Comment 19

[Brendan Eich [:brendan]]

wtc: I haven't kept up with Unix standards in years, but strncmp and strncpy
originated in the system 7 filesystem days for handling 14-char struct direct
(directory entry) filename arrays, which are NUL-padded to 14 chars, but not
necessarily NUL-terminated.  It seems to me that PL_strnchr (an extension?  man
strnchr on my RH6.1 system finds no man page) should work likewise.

IOW, dbaron is right and that for loop should test n before *s.  I call the
failure to do so a "violation of the PLA applied to strn* routines whose names
suggest they should work as strncmp and strncpy do" bug in PL_strnchr.  Agreed?

/be

Comment 20

[neeti]

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

Comment 21

[Fred Roeber]

The point of this function is that the string doesn't have to be
null-terminated.

Brendan is correct; it should test (n && *s) and not the other way around.
The long if (line 77) should probably be similarly amended.

Note that PL_strnrchr should be fixed too.

Unless some pre-mozilla hacking happened on this file (admittedly that
doesn't look like my style), this one is my fault.  Sorry.

Comment 22

[Wan-Teh Chang]

Brendan is right -- the arguments to the strn* routines don't need
to be strings.  You are all right.  I was wrong.

Comment 23

[Bradley Baetz (:bbaetz)]

VERIFIED via lxr. Was an nspr bug filed?

Comment 24

[Wan-Teh Chang]

No, I haven't filed an NSPR bug.

Comment 25

[Wan-Teh Chang]

OK, filed NSPR bug 96571.

Comment 26

[Bradley Baetz (:bbaetz)]

Filed as bug 96572.

Comment 27

[Bradley Baetz (:bbaetz)]

...which I just marked as a dupe of wtc's bug.