Last Comment Bug 716821 - crash MimeHeaders_build_heads_list even if bug 624419 is landed
: crash MimeHeaders_build_heads_list even if bug 624419 is landed
Status: RESOLVED FIXED
: crash
Product: MailNews Core
Classification: Components
Component: MIME (show other bugs)
: Trunk
: x86 Windows NT
: -- critical (vote)
: Thunderbird 13.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-10 00:23 PST by Makoto Kato [:m_kato]
Modified: 2012-02-13 06:27 PST (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (918 bytes, patch)
2012-01-26 13:03 PST, :aceman
mozilla: review-
Details | Diff | Splinter Review
A test (4.76 KB, text/plain)
2012-01-26 18:48 PST, Hiroyuki Ikezoe (:hiro)
no flags Details
patch v2 (933 bytes, patch)
2012-01-27 10:58 PST, :aceman
mozilla: review+
Details | Diff | Splinter Review

Description Makoto Kato [:m_kato] 2012-01-10 00:23:06 PST
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 2012-01-25 08:33:53 PST
Should I make a patch? Will you be able to test it?
Comment 2 Hiroyuki Ikezoe (:hiro) 2012-01-25 16:22:27 PST
(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 2012-01-26 02:19:18 PST
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 2012-01-26 13:03:16 PST
Created attachment 591901 [details] [diff] [review]
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] 2012-01-26 13:31:51 PST
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) 2012-01-26 15:24:43 PST
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) 2012-01-26 18:48:15 PST
Created attachment 592028 [details]
A test

Unfortunately this test can not be built since MimeHeaders_build_heads_list is not exported...
Comment 8 David :Bienvenu 2012-01-27 07:42:30 PST
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 2012-01-27 10:58:48 PST
Created attachment 592183 [details] [diff] [review]
patch v2
Comment 10 Hiroyuki Ikezoe (:hiro) 2012-01-30 02:14:46 PST
(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 2012-02-06 05:02:40 PST
Comment on attachment 592183 [details] [diff] [review]
patch v2

thx for the patch.
Comment 12 Mark Banner (:standard8) 2012-02-13 06:27:39 PST
Checked in: http://hg.mozilla.org/comm-central/rev/4e8a635051f2

Note You need to log in before you can comment on or make changes to this bug.