716821 - crash MimeHeaders_build_heads_list even if bug 624419 is landed

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Makoto Kato [:m_kato]]

MimeHeaders_build_heads_list still has overrun when boundary is \r and \n.

We should check "if (s+1 < end)" in the following.

313 if (*s == '\r')
314   s++;
315 if (*s == '\n')
316   s++; 

This bug was filed from the Socorro interface and is 
report bp-3b018667-f326-429a-8db7-bfc322120105 .
============================================================= 
0 	xul.dll 	MimeHeaders_build_heads_list 	mailnews/mime/src/mimehdrs.cpp:315
1 	xul.dll 	MimeHeaders_parse_line 	mailnews/mime/src/mimehdrs.cpp:154
2 	xul.dll 	MimeMessage_parse_line 	mailnews/mime/src/mimemsg.cpp:270
3 	xul.dll 	MimeMessage_parse_eof 	mailnews/mime/src/mimemsg.cpp:585
4 	xul.dll 	mime_display_stream_complete 	mailnews/mime/src/mimemoz2.cpp:1014
5 	xul.dll 	nsStreamConverter::OnStopRequest 	mailnews/mime/src/nsStreamConverter.cpp:1090
6 	xul.dll 	nsMsgProtocol::OnStopRequest 	mailnews/base/util/nsMsgProtocol.cpp:429
7 	xul.dll 	nsMailboxProtocol::OnStopRequest 	mailnews/local/src/nsMailboxProtocol.cpp:381
8 	xul.dll 	nsInputStreamPump::OnStateStop 	netwerk/base/src/nsInputStreamPump.cpp:578
9 	xul.dll 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:403
10 	xul.dll 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:114
11 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:631
12 	xul.dll 	NS_ProcessNextEvent_P 	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:245
13 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
14 	xul.dll 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:208
15 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:201
16 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:175
17 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
18 	xul.dll 	nsAppShell::Run 	widget/src/windows/nsAppShell.cpp:261
19 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:228
20 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3557
21 	thunderbird.exe 	do_main 	mail/app/nsMailApp.cpp:143
22 	thunderbird.exe 	NS_internal_main 	mail/app/nsMailApp.cpp:226
23 	thunderbird.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:107

Comment 1

[:aceman]

Should I make a patch? Will you be able to test it?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to :aceman from comment #1)
> Should I make a patch? Will you be able to test it?

Yes, please.
You can write the test something like mailnews/mime/test/TestMimeCrash.cpp does even if that test is not built for now.

Comment 3

[:aceman]

I don't know if this needs a synthetic test. I meant if you experience the real life crash and can test if the fix prevents it.

Comment 4

[:aceman]

Attachment: patch

I think this could be the fix but I can't test it. I don't know what the function does.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 591901 [details] [diff] [review]
patch

Bienvenu is the better reviewer for this since he reviewed the other patches in this bug's lineage.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 591901 [details] [diff] [review]
patch

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

::: mailnews/mime/src/mimehdrs.cpp
@@ +309,5 @@
>  
>      /* At this point, `s' points before a header-terminating newline.
>       Move past that newline, and store that new position in `heads'.
>       */
> +    if (s < end && *s == '\r')

This check is not needed since 's' is always lesser than 'end' here.

@@ +314,2 @@
>        s++;
> +    if (s < end && *s == '\n')

Actually I'd prefer to break here like the following 

if (s >= end)
  break;
if (*s == '\n')
  s++;

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test

Unfortunately this test can not be built since MimeHeaders_build_heads_list is not exported...

Comment 8

[David :Bienvenu]

Comment on attachment 591901 [details] [diff] [review]
patch

Hiro is right that the first change is not needed...aceman, can you submit a new patch and I'll review it right away, thx!

Hiro, for the test, would it be possible to #include mimehdrs.cpp into your test file, instead of copying the method?

Comment 9

[:aceman]

Attachment: patch v2

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to David :Bienvenu from comment #8)

> Hiro, for the test, would it be possible to #include mimehdrs.cpp into your
> test file, instead of copying the method?

Theoretically, yes. But if so, we need more and more methods in the source file. e.g. MimeOptions_write, mimeEmitterAddAttachmentField, etc. etc...

Comment 11

[David :Bienvenu]

Comment on attachment 592183 [details] [diff] [review]
patch v2

thx for the patch.

Comment 12

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/4e8a635051f2