crashes [@ PL_strnchr] called from ParseChunkRemaining

VERIFIED FIXED in mozilla0.9.2



Networking: HTTP
17 years ago
17 years ago


(Reporter: dbaron, Assigned: Darin Fisher)



Windows NT

Firefox Tracking Flags

(Not tracked)


(crash signature)


(1 attachment)



17 years ago
One of the top crashes listed in
is a crash with the following stack (lines are from 5-18 build): 

PL_strnchr [../../../../lib/libc/src/strchr.c line 61] 
line 84] 
line 55] 
line 377] 
line 589] 
 nsReadFromInputStream [d:\builds\seamonkey\mozilla\xpcom\io\nsPipe2.cpp line
[d:\builds\seamonkey\mozilla\xpcom\io\nsPipe2.cpp line 705] 
[d:\builds\seamonkey\mozilla\xpcom\io\nsPipe2.cpp line 839] 
 nsReadFromInputStream [d:\builds\seamonkey\mozilla\xpcom\io\nsPipe2.cpp line
line 177] 
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpConnection.cpp line

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

17 years ago
FWIW, the user comments are:

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

Comment 2

17 years ago
-> me
Assignee: neeti → darin

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.


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 and had gone to and clicked in each window.

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::HandleChunkedContent(char *,UINT,UINT *) 
        nsHttpTransaction::HandleContent(char *,UINT,UINT *) 
        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 *) 
        nsStreamListenerProxy::OnDataAvailable(nsIRequest *,nsISupports 
*,nsIInputStream *,UINT,UINT) [nsStreamListenerProxy.cpp:283]
        nsHttpTransaction::OnDataReadable(nsIInputStream *) 

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

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.

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.

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.

Comment 13

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

Comment 14

17 years ago
we should probably aim to get this in for 0.9.1
Keywords: nsbeta1
Target Milestone: --- → mozilla0.9.2

Comment 15

17 years ago

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.

Comment 18

17 years ago
fix checked in on the trunk and the 0.9.1 branch.
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?


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

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.


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

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.