crash deleting mail [@ MimeHeaders_write_all_headers - MSVCRT.DLL]

VERIFIED FIXED

Status

MailNews Core
MIME
P3
major
VERIFIED FIXED
18 years ago
10 years ago

People

(Reporter: jay, Assigned: rhp (gone))

Tracking

({crash, topcrash})

Trunk
x86
Windows 2000
crash, topcrash

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm++], crash signature)

(Reporter)

Description

18 years ago
This is a topcrash for PR2 build 2000080712 according to talkback, but i am not 
able to reproduce. Data shows this occurring on Win95 and Win98.  The stack 
traces point to this location (assigning to rhp, cvs blame says it's his):
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/mime/src/mimehdrs.c
pp&rev=SeaMonkey_M17_BRANCH&mark=783#773

Here is a stack trace and some talkback entries for this crash:
Incident ID 16059813 
MSVCRT.DLL + 0x1648 (0x78001648) 
MimeHeaders_write_all_headers 
[d:\builds\seamonkey\mozilla\mailnews\mime\src\mimehdrs.cpp, line 784] 
MimeMessage_write_headers_html 
[d:\builds\seamonkey\mozilla\mailnews\mime\src\mimemsg.cpp, line 715] 
MimeMessage_close_headers 
[d:\builds\seamonkey\mozilla\mailnews\mime\src\mimemsg.cpp, line 390] 
MimeMessage_parse_line 
[d:\builds\seamonkey\mozilla\mailnews\mime\src\mimemsg.cpp, line 256] 
convert_and_send_buffer 
[d:\builds\seamonkey\mozilla\mailnews\mime\src\mimebuf.cpp, line 123] 
mime_LineBuffer [d:\builds\seamonkey\mozilla\mailnews\mime\src\mimebuf.cpp, line 
245] 
MimeObject_parse_buffer 
[d:\builds\seamonkey\mozilla\mailnews\mime\src\mimeobj.cpp, line 254] 
MimeMessage_parse_line 
[d:\builds\seamonkey\mozilla\mailnews\mime\src\mimemsg.cpp, line 131] 
nsStreamConverter::OnDataAvailable 
[d:\builds\seamonkey\mozilla\mailnews\mime\src\nsStreamConverter.cpp, line 871] 
nsStreamConverter::OnDataAvailable 
[d:\builds\seamonkey\mozilla\mailnews\mime\src\nsStreamConverter.cpp, line 871] 
nsDocumentOpenInfo::OnDataAvailable 
[d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp, line 252] 
nsMailboxProtocol::ReadMessageResponse 
[d:\builds\seamonkey\mozilla\mailnews\local\src\nsMailboxProtocol.cpp, line 433] 
nsMailboxProtocol::ProcessProtocolState 
[d:\builds\seamonkey\mozilla\mailnews\local\src\nsMailboxProtocol.cpp, line 467] 
nsMsgProtocol::OnDataAvailable 
[d:\builds\seamonkey\mozilla\mailnews\base\util\nsMsgProtocol.cpp, line 193] 
nsFileChannel::OnDataAvailable 
[d:\builds\seamonkey\mozilla\netwerk\protocol\file\src\nsFileChannel.cpp, line 
665] 
nsOnDataAvailableEvent::HandleEvent 
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsAsyncStreamListener.cpp, line 
406] 
nsStreamListenerEvent::HandlePLEvent 
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsAsyncStreamListener.cpp, line 
106] 
PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 588] 
PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, 
line 547] 
_md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 
1045] 
KERNEL32.DLL + 0x363b (0xbff7363b) 
KERNEL32.DLL + 0x24407 (0xbff94407) 
0x0068831e 

MSVCRT.DLL + 0x1648 (0x78001648) 9170e8d2
         line 
        Build: 2000080712 CrashDate: 2000-08-21 UptimeMinutes: 17  Total: 17 
        OS: Windows 98  4.10 build 67766446
        URL: 
        Comment: i was sending email and reading email
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=16059813

MSVCRT.DLL + 0x1648 (0x78001648) 0ae4b692
         line 
        Build: 2000080712 CrashDate: 2000-08-21 UptimeMinutes: 612  Total: 667 
        OS: Windows 98  4.10 build 67766446
        URL: Mail Viewer
        Comment: 
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=16080030
(Reporter)

Comment 1

18 years ago
adding topcrash keyword.  
Depends on: 50026
Keywords: topcrash

Comment 2

18 years ago
Are there any instances at all of this crash in M18 talkback incidents?  I am
curioius to know if we need to devote time to reproduce this (doesn't look like
very good reproducible steps).  If there are NO M18 talkback incidents, then
perhaps this bug may have been fixed by the many checkins since M17.

Severity: normal → major
Keywords: crash
Whiteboard: [need repro]
Still shows up in current builds, e.g. following from 8-23:
http://cyclone/reports/incidenttemplate.cfm?bbid=16245153

Comment 4

18 years ago
Am I missing something?  I looked at 
http://cyclone/reports/incidenttemplate.cfm?bbid=16245153, but I don't see on 
the report where the build used is specified.

Peter - another one to put on a list to try to reproduce.  Perhaps contact the 
reporter in the incident report.

Nominate nsbeta3 to get on mailtriage radar.
Keywords: nsbeta3
QA Contact: lchiang → pmock

Comment 5

18 years ago
Never mind - I see the build ID on the incident report.
(Assignee)

Comment 6

18 years ago
Please attach the message that causes this crash to the bug report.

- rhp
(Assignee)

Comment 7

18 years ago
Tried a bunch of things...seems to work.

- rhp
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → WORKSFORME

Comment 8

18 years ago
Jay,  Are you still experiencing this crash?  I am trying to verify this bug and 
I have not been able to reproduce but wanted to check with you before closing 
this bug.
(Reporter)

Comment 9

18 years ago
Peter, I have not seen this crash as much as before in the latest talkback 
data...but I did find a couple of entries for a recent branch build.  Both 
crashes were sent in by the same person, so it might be a good idea to inspect 
the talkback incidents for more info and email the user for any specific steps 
to reproduce.

MSVCRT.DLL + 0x11ce (0x780011ce) 618c5539
         line 
        Build: 2000092909 CrashDate: 2000-09-30 UptimeMinutes: 135  Total: 135 
        OS: Windows NT  5.0 build 2195
        URL: 
        Comment: mail
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=18320088
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=18320088

 MSVCRT.DLL + 0x11ce (0x780011ce) 618c5539
         line 
        Build: 2000092909 CrashDate: 2000-09-30 UptimeMinutes: 0  Total: 136 
        OS: Windows NT  5.0 build 2195
        URL: 
        Comment: 
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=18320107
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=18320107

It definitely is no longer a "topcrash"...but from these entries it is clear 
that someone is still crashing.
(Reporter)

Comment 10

18 years ago
I'm reopening bug, since someone is still crashing with this stack trace.  I am 
also updating the summary.  Here are some very recent crashes:

MSVCRT.DLL + 0x11ce (0x780011ce) 618c5539
         line 
        Build: 2000100309 CrashDate: 2000-10-03 UptimeMinutes: 4  Total: 4 
        OS: Windows NT  5.0 build 2195
        URL: 
        Comment: Mail .. downloaded a lot and then deleting and it crashed ... 
yuck .. mail getting unstable again? it was better .. and is getting better 
overall ... but ?
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=18457831
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=18457831

    MSVCRT.DLL + 0x11ce (0x780011ce) 618c5539
         line 
        Build: 2000100309 CrashDate: 2000-10-03 UptimeMinutes: 0  Total: 4 
        OS: Windows NT  5.0 build 2195
        URL: 
        Comment: mail again .... crashing when deleting .. seems to be a timing 
issue perhaps ...
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=18457903
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=18457903

    MSVCRT.DLL + 0x11ce (0x780011ce) 618c5539
         line 
        Build: 2000100309 CrashDate: 2000-10-03 UptimeMinutes: 4  Total: 9 
        OS: Windows NT  5.0 build 2195
        URL: 
        Comment: mail sucks ...... not really .. but deleting it does ...
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=18458308
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=18458308

Peter, try reproducing using these comments and if u still think this is worth a 
worksforme...then mark it so again.  NOTE that these latest crashes are on 
Windows 2000.
Status: RESOLVED → REOPENED
OS: All → Windows 2000
Resolution: WORKSFORME → ---
Summary: crash reading and sending mail (mimehdrs.cpp, line 783) [@ MSVCRT.DLL ] → crash deleting mail [@ MSVCRT.DLL - MimeHeaders_write_all_headers(mimehdrs.cpp, line 827)]
(Assignee)

Comment 11

18 years ago
Sorry, but I haven't seen this one once recently.

- rhp
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → WORKSFORME
Reopening.  This still shows up in talkback reports for 10-08, 10-09, 10-11, and
10-12.  See http://www.mozilla.org/projects/seamonkey/reports/ns6analysis.html

You seem to be crashing on the memcpy here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/mime/src/mimehdrs.cpp&rev=1.40&mark=826#816

I wonder if end could be less than contents.  (That can happen as a result of
the first loop.)  What does memcpy do then?  (And what does PR_MALLOC do?)
Also, could memcpy do something strange if given 0 bytes to copy (hopefully not)?

It might be a good idea to try and bullet-proof this code a little more and see
if these talkback reports go away.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Updated

18 years ago
Status: REOPENED → ASSIGNED
(Reporter)

Updated

18 years ago
Summary: crash deleting mail [@ MSVCRT.DLL - MimeHeaders_write_all_headers(mimehdrs.cpp, line 827)] → crash deleting mail [@ MimeHeaders_write_all_headers - MSVCRT.DLL]
(Assignee)

Comment 13

18 years ago
There is a very good chance this is the same problem as:

    http://bugzilla.mozilla.org/show_bug.cgi?id=57059

That was fixed by jefft yesterday. This looks a lot like a memory corruption 
problem which is what jefft was dealing with.

- rhp
This crash is still occurring in trunk builds as recently as 10-25.

Comment 15

18 years ago
This is showing up as one of our top crashes of the last 3 days as well.
Keywords: rtm
(Assignee)

Comment 16

18 years ago
Ok...will re-investigate...again ;-)

- rhp

Comment 17

18 years ago
I agree with David Baron's suggestion. If we are dying on the memcpy to a newly
allocated piece of memory, I'm wondering if we should verify that end-contents >
0. I don't know why we'd get in this case.  FYI, the most recent crash is from
the 10/29 build talkback id:
http://climate/reports/incidenttemplate.cfm?bbid=20028236

Comment 18

18 years ago
I haven't reproduced this on my own, but I can force it to happen in the
debugger by making "end" be one less than "content".

    /* Skip over whitespace after colon. */
    while (contents <= end && nsCRT::IsAsciiSpace(*contents))
      contents++;

    /* Take off trailing whitespace... */
    while (end > contents && nsCRT::IsAsciiSpace(end[-1]))
      end--;

In theory, after the first while loop it could get in this state since it will
hit the statement contents++ if end==contents.

(Assignee)

Comment 19

18 years ago
Well, I can certainly do something like:

Index: mimehdrs.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/mime/src/mimehdrs.cpp,v
retrieving revision 1.40
diff -u -r1.40 mimehdrs.cpp
--- mimehdrs.cpp	2000/09/13 23:53:42	1.40
+++ mimehdrs.cpp	2000/10/30 18:28:58
@@ -817,14 +817,17 @@
     nsCRT::memcpy(name, head, colon - head);
     name[colon - head] = 0;
     
-    c2 = (char *)PR_MALLOC(end - contents + 1);
-    if (!c2)
+    if ((end - contents) > 0)
     {
-      PR_Free(name);
-      return MIME_OUT_OF_MEMORY;
+      c2 = (char *)PR_MALLOC(end - contents + 1);
+      if (!c2)
+      {
+        PR_Free(name);
+        return MIME_OUT_OF_MEMORY;
+      }
+      nsCRT::memcpy(c2, contents, end - contents);
+      c2[end - contents] = 0;
     }
-    nsCRT::memcpy(c2, contents, end - contents);
-    c2[end - contents] = 0;
     
     if (attachment)
       status = mimeEmitterAddAttachmentField(opt, name, 


Just to make it more bulletproof. I have the change in my tree and running.

Any thoughts?

- rhp
Makes sense to me.  Might you also want to change |contents <= end| to |contents
< end|, since there isn't much point making contents more than end (that might
be why we're crashing now anyway)?  (It might even be nice to write |end >
contents| in that order to show they are the same condition in both |while| loops.)

Comment 21

18 years ago
I bet this will do it.  Like I said earlier, the only way I could crash it was
to make the ptrs in the state where end was less than content, which is
possible.  If this happens.  You'll have to check the PR_FREE(c2) later on to
make sure that it's been allocated.  Will anything bad happen in the rest of the
function if c2 isn't allocated(I see it gets passed to the attachment code right
below).
(Assignee)

Comment 22

18 years ago
All good suggestions, but because of where we are in the release process, I 
want to just stay with my very simple to understand sanity check. Just 
basically adding on easy to understand line instead of changing stuff in more 
than one place.

- rhp

Comment 23

18 years ago
David makes a good point. Unless there's a reason not to, you could probably
just change the while loop so that at worst contents==end and then you wouldn't
have to make any changes to the rest of the code.

I can see why you might be worried about changing the while loop, but if you go
with the sanity check, I think you need to worry about the other things I
mention or else c2 will not be an allocated pointer when you try to free it.
(Assignee)

Comment 24

18 years ago
Ok...then:

Index: mimehdrs.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/mime/src/mimehdrs.cpp,v
retrieving revision 1.40
diff -u -r1.40 mimehdrs.cpp
--- mimehdrs.cpp	2000/09/13 23:53:42	1.40
+++ mimehdrs.cpp	2000/10/30 18:56:40
@@ -805,7 +805,7 @@
     }
     
     /* Skip over whitespace after colon. */
-    while (contents <= end && nsCRT::IsAsciiSpace(*contents))
+    while (contents < end && nsCRT::IsAsciiSpace(*contents))
       contents++;
     
     /* Take off trailing whitespace... */
After looking at this again, what says contents is initialized *at all*?

Comment 26

18 years ago
It's hard to say why it's crashing without being able to reproduce it. Either,
it's the while loop we've been talking about or it's some case where "head" ==
"end" and we never get into the code that sets "content".

I'm wondering if we should initialize "content=end" to make sure it has a value
and then still add rhp's safety checks that he just took out.
(Assignee)

Comment 27

18 years ago
Ok fellas...how about this one:

Index: mimehdrs.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/mime/src/mimehdrs.cpp,v
retrieving revision 1.40
diff -u -r1.40 mimehdrs.cpp
--- mimehdrs.cpp	2000/09/13 23:53:42	1.40
+++ mimehdrs.cpp	2000/10/30 19:21:16
@@ -769,6 +769,8 @@
 	  if (status < 0) return 0;
 	}
 
+  char *c2;
+
   for (i = 0; i < hdrs->heads_size; i++)
   {
     char *head = hdrs->heads[i];
@@ -776,9 +778,9 @@
                       ? hdrs->all_headers + hdrs->all_headers_fp
                       : hdrs->heads[i+1]);
     char *colon, *ocolon;
-    char *contents;
+    char *contents = end;
     char *name = 0;
-    char *c2 = 0;
+    c2 = 0;
     
     /* Hack for BSD Mailbox delimiter. */
     if (i == 0 && head[0] == 'F' && !nsCRT::strncmp(head, "From ", 5))
@@ -805,7 +807,7 @@
     }
     
     /* Skip over whitespace after colon. */
-    while (contents <= end && nsCRT::IsAsciiSpace(*contents))
+    while (contents < end && nsCRT::IsAsciiSpace(*contents))
       contents++;
     
     /* Take off trailing whitespace... */
@@ -816,15 +818,18 @@
     if (!name) return MIME_OUT_OF_MEMORY;
     nsCRT::memcpy(name, head, colon - head);
     name[colon - head] = 0;
-    
-    c2 = (char *)PR_MALLOC(end - contents + 1);
-    if (!c2)
+  
+    if ( (end - contents) > 0 )
     {
-      PR_Free(name);
-      return MIME_OUT_OF_MEMORY;
+      c2 = (char *)PR_MALLOC(end - contents + 1);
+      if (!c2)
+      {
+        PR_Free(name);
+        return MIME_OUT_OF_MEMORY;
+      }
+      nsCRT::memcpy(c2, contents, end - contents);
+      c2[end - contents] = 0;
     }
-    nsCRT::memcpy(c2, contents, end - contents);
-    c2[end - contents] = 0;
     
     if (attachment)
       status = mimeEmitterAddAttachmentField(opt, name, 
@@ -834,7 +839,7 @@
                                 MimeHeaders_convert_header_value(opt, &c2));
 
     PR_Free(name);
-    PR_Free(c2);
+    PR_FREEIF(c2);
     
     if (status < 0) return status;
     if (!wrote_any_p) 

Comment 28

18 years ago
cool. this seems like total bulletproofing.

MimeHeaders_convert_header_value does the right thing when c2 is null by just
returning null.  I haven't looked at mimeEmitterAddHeaderField or
mimeEmitterAddAttachmentField but hopefully they do the same thing.

Comment 29

18 years ago
let's try to get this on the rtm+ list then soon :-)
I'll mark rtm need info for now since a fix is close.
Whiteboard: [need repro] → [need repro][rtm need info]
(Assignee)

Comment 30

18 years ago
Agreed. The final patch is the one we should go with!

- rhp

Comment 31

18 years ago
we need to get a super review on this first.

Comment 32

18 years ago
sr=mscott

Comment 33

18 years ago
moving to rtm+
Whiteboard: [need repro][rtm need info] → [need repro][rtm+]
(Assignee)

Comment 34

18 years ago
Just checked this into the branch.

- rhp

Comment 35

18 years ago
the branch or the trunk? This didn't get an rtm++ from pdt.
(Assignee)

Comment 36

18 years ago
Oh, geeze...sorry...the Trunk...NOT the branch.

- rhp

Comment 37

18 years ago
This bug is in candidate limbo.

Comment 38

18 years ago
rtm++, please land on branch ASAP.
Whiteboard: [need repro][rtm+] → [rtm++]
(Assignee)

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 39

18 years ago
All done!

- rhp

Comment 40

18 years ago
We still don't have a good scenario for testing this, could someone who
reproduced this problem, verify this bug or give some steps to test this.  I
can't find steps in the talkback reports that are still active.
I think the way to verify this is to see if the talkback reports stop.

Comment 42

18 years ago
OK, will watch for it in talkback reports.

Comment 43

18 years ago
I haven't seen any current talkback incidents with this crash.  Verifying,
please reopen if you find this still happens.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
Crash Signature: [@ MimeHeaders_write_all_headers - MSVCRT.DLL]
You need to log in before you can comment on or make changes to this bug.