50239 - crash deleting mail [@ MimeHeaders_write_all_headers - MSVCRT.DLL]

Status: VERIFIED FIXED

(MailNews Core :: MIME, defect, P3)

Description

[Jay Patel [:jay]]

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

Comment 1

[Jay Patel [:jay]]

adding topcrash keyword.  

Comment 2

[lchiang]

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.

Comment 3

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

Still shows up in current builds, e.g. following from 8-23:
http://cyclone/reports/incidenttemplate.cfm?bbid=16245153

Comment 4

[lchiang]

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.

Comment 5

[lchiang]

Never mind - I see the build ID on the incident report.

Comment 6

[rhp (gone)]

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

- rhp

Comment 7

[rhp (gone)]

Tried a bunch of things...seems to work.

- rhp

Comment 8

[pmock]

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.

Comment 9

[Jay Patel [:jay]]

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.

Comment 10

[Jay Patel [:jay]]

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.

Comment 11

[rhp (gone)]

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

- rhp

Comment 12

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

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.

Comment 13

[rhp (gone)]

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

Comment 14

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

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

Comment 15

[scottputterman]

This is showing up as one of our top crashes of the last 3 days as well.

Comment 16

[rhp (gone)]

Ok...will re-investigate...again ;-)

- rhp

Comment 17

[scottputterman]

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

[scottputterman]

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.

Comment 19

[rhp (gone)]

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

Comment 20

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

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

[scottputterman]

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).

Comment 22

[rhp (gone)]

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

[scottputterman]

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.

Comment 24

[rhp (gone)]

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... */

Comment 25

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

After looking at this again, what says contents is initialized *at all*?

Comment 26

[scottputterman]

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.

Comment 27

[rhp (gone)]

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

[scottputterman]

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

[lchiang]

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.

Comment 30

[rhp (gone)]

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

- rhp

Comment 31

[scottputterman]

we need to get a super review on this first.

Comment 32

[Scott MacGregor]

sr=mscott

Comment 33

[scottputterman]

moving to rtm+

Comment 34

[rhp (gone)]

Just checked this into the branch.

- rhp

Comment 35

[scottputterman]

the branch or the trunk? This didn't get an rtm++ from pdt.

Comment 36

[rhp (gone)]

Oh, geeze...sorry...the Trunk...NOT the branch.

- rhp

Comment 37

[selmer (gone)]

This bug is in candidate limbo.

Comment 38

[selmer (gone)]

rtm++, please land on branch ASAP.

Comment 39

[rhp (gone)]

All done!

- rhp

Comment 40

[esther]

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.

Comment 41

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

I think the way to verify this is to see if the talkback reports stop.

Comment 42

[esther]

OK, will watch for it in talkback reports.

Comment 43

[esther]

I haven't seen any current talkback incidents with this crash.  Verifying,
please reopen if you find this still happens.