469087 - Open draft message cause a crash [@ mime_decode_qp_buffer ]

Status: RESOLVED INCOMPLETE

(MailNews Core :: MIME, defect)

Description

[Nikolay Shopik]

Look for attachment which is cause such crash when it lies in drafts and you try to edit it will crash TB. You may notice message is kinda wrong corrupted header, I've never touched message it lies in drafts probably 3-4 months so probably one of nigthies corrupted it before.
bp-c179c43f-5715-4cd4-a111-983fd2081211
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081210 Shredder/3.0b2pre

Comment 1

[Nikolay Shopik]

Attachment: draft message with wrong header

Comment 2

[Phil Ringnalda (:philor)]

0  	thunderbird.exe  	mime_decode_qp_buffer  	mimeenc.cpp:193
1 	thunderbird.exe 	MimeDecoderWrite 	mimeenc.cpp:838
2 	thunderbird.exe 	mime_decompose_file_output_fn 	mimedrft.cpp:1966
3 	thunderbird.exe 	MimeMessage_parse_line 	mimemsg.cpp:230
4 	thunderbird.exe 	MimeObject_parse_eof 	mimeobj.cpp:300
5 	thunderbird.exe 	MimeContainer_parse_eof 	mimecont.cpp:129
6 	thunderbird.exe 	MimeMessage_parse_eof 	mimemsg.cpp:550
7 	thunderbird.exe 	mime_parse_stream_complete 	mimedrft.cpp:1239
8 	thunderbird.exe 	nsStreamConverter::OnStopRequest 	nsStreamConverter.cpp:1049
9 	thunderbird.exe 	nsImapCacheStreamListener::OnStopRequest 	nsImapProtocol.cpp:8071
10 	thunderbird.exe 	nsInputStreamPump::OnStateStop 	netwerk/base/src/nsInputStreamPump.cpp:576
11 	thunderbird.exe 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:401
12 	xpcom_core.dll 	nsOutputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:111
13 	xpcom_core.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
14 	xpcom_core.dll 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:227
15 	thunderbird.exe 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:170
16 	thunderbird.exe 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:192
17 	thunderbird.exe 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3264
18 	thunderbird.exe 	NS_internal_main 	nsMailApp.cpp:103
19 	thunderbird.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:87
20 	thunderbird.exe 	__tmainCRTStartup 	crtexe.c:594
21 	kernel32.dll 	kernel32.dll@0x17066

Comment 3

[David :Bienvenu]

taking - I'll try to recreate and fix this soon.

Comment 4

[David :Bienvenu]

this doesn't crash on my build. I had to tweak it slightly to get the eml file into a mailbox format by sticking "From " at the beginning.

Comment 5

[Nikolay Shopik]

It doesn't crash if I just open eml file. Only when I try to open it from drafts

Comment 6

[David :Bienvenu]

this still doesn't crash for me when editing as a draft, for either imap or local. So, moving to b3. Can you still reproduce this, Nikolay?

Comment 7

[Nikolay Shopik]

I can't copy it to my IMAP drafts folder, so I can't reproduce it.

Comment 8

[David :Bienvenu]

it looks like it was in an imap folder before when it crashed - why can't you copy it to your imap drafts folder?

It also looks like you didn't have the message offline, from the stack - I'll see if I can get that to happen...

Comment 9

[Nikolay Shopik]

I'm on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090109 Shredder/3.0b2pre trying drag and drop file to drafts folder from my desktop, nothing happens.

Comment 10

[David :Bienvenu]

that has never worked, afaik. To get it into an imap folder, you first need to convert the .eml file to a local mailbox by putting a "From " line at the top, put it in your profile dir under a local mail account, and then copy the message to an imap folder. That being said, I tried that and could not recreate the crash.

Comment 11

[David :Bienvenu]

I see 8 of these crashes in b2, but none in b3pre - Nikolay, do you still see this crash?

Comment 12

[David :Bienvenu]

I'm going to move this off the blocker list until we can find a way to reproduce it...

Comment 13

[Nikolay Shopik]

Nope I don't see it in latest nightly, but somehow I'm still getting corrupted drafts messages. This is how this message comes to my computer it's just many months old draft. Rebuilding index seems like get it in shape again.

Comment 14

[Nikolay Shopik]

(In reply to comment #13)
Therefore I see no reason to keep this open for now.

Comment 15

[Ludovic Hirlimann [:Usul]]

As they still appear

Comment 16

[Ludovic Hirlimann [:Usul]]

http://crash-stats.mozilla.com/report/index/32b2cf82-a249-4333-a004-e392e2090717

Comment 17

[David :Bienvenu]

moving out of b3, then.

Comment 18

[Wayne Mery (:wsmwk)]

found 3 comment in last 4 months

When trying to display an e-mail message, i did'nt see the one i had selected but rather the prior emai i had viewed/deleted. This occured even after a restart and the deleted email deleted. 
bp-5876fd45-d041-43fb-b0f1-ffd0a2091101 3.0pre

Presssed the "Forward" button from the top bar. Just after that thunderbird went to heaven.
bp-165103b1-63a3-466e-80eb-96a7c2090708

click on the message to forward, it just crashes
bp-78fb78f8-5658-447d-b5d9-557e92090612

Comment 19

[Nikolay Shopik]

Looks like it still there about 202 crashes last month
http://crash-stats.mozilla.com/report/list?product=Thunderbird&query_search=signature&query_type=exact&query=mime_decode_qp_buffer&date=05%2F12%2F2010%2012%3A15%3A54&range_value=4&range_unit=weeks&process_type=all&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&signature=mime_decode_qp_buffer

Comment 20

[Wayne Mery (:wsmwk)]

Nikolay, are you still unable to reproduce? Yeah, this is in top 100.

Makoto, do you see an obvious way to reproduce? 

Forward is still a common comment, but none of the commenters have left an email address from which we might get detailed steps or testcase. https://crash-stats.mozilla.com/report/list?product=Thunderbird&query_search=signature&query_type=exact&query=mime_decode_qp_buffer&date=12%2F30%2F2010%2006%3A14%3A17&range_value=4&range_unit=weeks&hang_type=any&process_type=all&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&admin=&signature=mime_decode_qp_buffer

Comment 21

[Nikolay Shopik]

I never seen crash in years

Comment 22

[Makoto Kato [:m_kato]]

This bug is MimeMessage_parse_line.  decompose_file_output_fn should not use const buffer.  (

Also, I think that writing test case is difficult since we cannot call mime API directly from test harness.

Comment 23

[Makoto Kato [:m_kato]]

Attachment: fix v1

Should I change all const char* define to char*?  If so, patch will be too large.

Comment 24

[David :Bienvenu]

Thx for looking at this.

(In reply to comment #23)
> Created attachment 502763 [details] [diff] [review]
> fix v1
> 
> Should I change all const char* define to char*?  If so, patch will be too
> large.

Yeah, you can't do that. I think the better approach would be stop the MimeDecoderWrite functions from decoding in place, i.e.,  mime_decode_base64_buffer and mime_decode_qp_buffer should decode into data->line_buffer, which we should alloc or realloc to be at least length bytes, similar to what the decode_uue and decode_yenc functions do, e.g.,

have a helper function that does this:

  if (!data->line_buffer)
  {
    data->line_buffer_size = length; // or perhaps something like NS_MAX(length, 100)
    data->line_buffer = (char *)PR_MALLOC(data->line_buffer_size);
    if (!data->line_buffer)
      return -1;
    data->line_buffer[0] = 0;
  }
  else if (data->line_buffer_size < length)
  {
    data->line_buffer = (char *) PR_REALLOC(data->line_buffer, length);
    if (!data->line_buffer)
      return -1;
    data->line_buffer_size = length;
  }

Does that make sense?

Comment 25

[David :Bienvenu]

Comment on attachment 502763 [details] [diff] [review]
fix v1

I'd prefer the approach I described earlier, so minusing this. Thx again for working on this.

Comment 26

[Makoto Kato [:m_kato]]

Attachment: fix v2

Comment 27

[David :Bienvenu]

Comment on attachment 504389 [details] [diff] [review]
fix v2

sorry for the delay, and sorry I wasn't clear earlier.

I was hoping you could do the same thing for mime_decode_base64_buffer, since I believe it has the same issue. And if so, then you would want to have a helper routine to do the allocation of data->line_buffer, so the code could be shared between mime_decode_base64_buffer and mime_decode_qp_buffer. Also, you should use PR_REALLOC instead of free and alloc, because it can be cheaper in some situations.

Finally, we don't do this braces style:

+  } else if (data->line_buffer_size < length) {

the else part should be on a separate line

Comment 28

[Wayne Mery (:wsmwk)]

Makoto Kato, are you up for doing patch v3?

Comment 29

[Wayne Mery (:wsmwk)]

(In reply to Wayne Mery (:wsmwk) from comment #28)
> Makoto Kato, are you up for doing patch v3?

If not, we'll foist, I mean, free it up for someone else :)

most crash comments mention forwarding a message.
bp-b84730c8-e8f0-41c5-8a8e-761c62121011 is TB15.0.1 example

Comment 30

[Makoto Kato [:m_kato]]

root cause is that const buffer isn't writable.  see comment #22

Comment 31

[Makoto Kato [:m_kato]]

Attachment: fix v3

Comment 32

[Joshua Cranmer [:jcranmer]]

Comment on attachment 676064 [details] [diff] [review]
fix v3

Review of attachment 676064 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/mime/src/mimeenc.cpp
@@ +41,5 @@
> +static void
> +AllocLineBuffer(MimeDecoderData *data, size_t length)
> +{
> +  if (length < 1000)
> +    length = 1000;

What's the purpose of this bit?

@@ +52,5 @@
> +      return;
> +    }
> +    data->line_buffer = (char *) PR_REALLOC(data->line_buffer, length);
> +  } else {
> +    data->line_buffer = (char *) PR_MALLOC(length);

Nit: s/PR_REALLOC/PR_Realloc/ and s/PR_MALLOC/PR_Malloc/

@@ +350,5 @@
>     it out.
>  
>     Then pull the next line into line_buffer and continue.
>     */
> +  AllocLineBuffer(data, input_length);

This isn't an equivalent change. AllocLineBuffer(data, 128) would be the equivalent change.

@@ +580,5 @@
>     it out.
>  
>     Then pull the next line into line_buffer and continue.
>     */
> +  AllocLineBuffer(data, input_length);

Same here. AllocLineBuffer(data, 1000) instead.

Comment 33

[Scoobidiver (away)]

The topcrash- keyword is not actively maintained and pollutes queries with topcrash.

Comment 34

[Wayne Mery (:wsmwk)]

(In reply to Joshua Cranmer [:jcranmer] from comment #32)
> Comment on attachment 676064 [details] [diff] [review]
> fix v3
> 
> Review of attachment 676064 [details] [diff] [review]:
> -----------------------------------------------------------------

Comment 35

[Wayne Mery (:wsmwk)]

easy to finish the proposed patch?

Comment 36

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 37

[Wayne Mery (:wsmwk)]

m_kato, attachment 422879 [details] from bug 469087 also produces crash

Comment 38

[Makoto Kato [:m_kato]]

root causes is comment #21 and comment #22 due to const cast.   Reproduce step depends on memory condition, so it is too difficult as unit test and step

I am not interesting this bug due to rare crash, so please feel free to take this bug.  (I think that all mime code should be re-written by JavaScript)

Comment 39

[Wayne Mery (:wsmwk)]

I had delayed updating this hoping by now that we'd be fully on jsmime

Still with us, at least in version 60 bp-2a185a47-db6d-4413-912b-c26400190928
0 xul.dll mime_decode_qp_buffer comm/mailnews/mime/src/mimeenc.cpp:160
1 xul.dll MimeDecoderWrite(MimeDecoderData*, char const*, int, int*) comm/mailnews/mime/src/mimeenc.cpp:834
2 xul.dll mime_decompose_file_output_fn(char const*, int, void*) comm/mailnews/mime/src/mimedrft.cpp:2135
3 xul.dll MimeMessage_parse_line comm/mailnews/mime/src/mimemsg.cpp:200
4 xul.dll MimeObject_parse_eof comm/mailnews/mime/src/mimeobj.cpp:270
5 xul.dll MimeContainer_parse_eof comm/mailnews/mime/src/mimecont.cpp:97
6 xul.dll MimeMessage_parse_eof comm/mailnews/mime/src/mimemsg.cpp:565
7 xul.dll mime_parse_stream_complete comm/mailnews/mime/src/mimedrft.cpp:1287

Comment 40

[Wayne Mery (:wsmwk)]

No version 68 crashes, they end after 60.9.1.