crashes [@ PL_strnchr] called from ParseChunkRemaining

VERIFIED FIXED in mozilla0.9.2

Status

()

Core
Networking: HTTP
--
critical
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: dbaron, Assigned: Darin Fisher)

Tracking

({topcrash})

Trunk
mozilla0.9.2
x86
Windows NT
topcrash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

17 years ago
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).
(Reporter)

Comment 1

17 years ago
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 
(Assignee)

Comment 2

17 years ago
-> me
Assignee: neeti → darin
(Reporter)

Comment 3

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

Updated

17 years ago
Keywords: topcrash

Comment 4

17 years ago
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.
(Reporter)

Comment 5

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

Comment 6

17 years ago
[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]
(Reporter)

Comment 7

17 years ago
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

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

CCing waterson.

Comment 9

17 years ago
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.
(Reporter)

Comment 10

17 years ago
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

17 years ago
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.
(Assignee)

Comment 12

17 years ago
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.
(Assignee)

Comment 13

17 years ago
Created attachment 37108 [details] [diff] [review]
fixes the problem
(Assignee)

Comment 14

17 years ago
we should probably aim to get this in for 0.9.1
Status: NEW → ASSIGNED
Keywords: nsbeta1
Target Milestone: --- → mozilla0.9.2
(Reporter)

Comment 15

17 years ago
r=dbaron

Agreed on 0.9.1, although that requires working fast.

Comment 16

17 years ago
r=gagan and check with chofmann on getting this in for 0.9.1 (small fix, low 
risk, high gain)
a=blizzard for 0.9.1 for drivers since no one actually put that in the bug.
(Assignee)

Comment 18

17 years ago
fix checked in on the trunk and the 0.9.1 branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
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

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

Comment 21

17 years ago
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

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

Updated

17 years ago
QA Contact: benc → bbaetz
VERIFIED via lxr. Was an nspr bug filed?
Status: RESOLVED → VERIFIED

Comment 24

17 years ago
No, I haven't filed an NSPR bug.

Comment 25

17 years ago
OK, filed NSPR bug 96571.
Filed as bug 96572.
...which I just marked as a dupe of wtc's bug.
Crash Signature: [@ PL_strnchr]
You need to log in before you can comment on or make changes to this bug.