Closed Bug 716821 Opened 12 years ago Closed 12 years ago

crash MimeHeaders_build_heads_list even if bug 624419 is landed


(MailNews Core :: MIME, defect)

Windows NT
Not set


(Not tracked)

Thunderbird 13.0


(Reporter: m_kato, Assigned: aceman)


(Keywords: crash)

Crash Data


(2 files, 1 obsolete file)

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/
15 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/
16 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/
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
Component: General → MIME
Product: Thunderbird → MailNews Core
QA Contact: general → mime
Should I make a patch? Will you be able to test it?
(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.
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.
Attached patch patch (obsolete) — Splinter Review
I think this could be the fix but I can't test it. I don't know what the function does.
Assignee: nobody → acelists
Attachment #591901 - Flags: review?(bugmail)
Comment on attachment 591901 [details] [diff] [review]

Bienvenu is the better reviewer for this since he reviewed the other patches in this bug's lineage.
Attachment #591901 - Flags: review?(bugmail) → review?(dbienvenu)
Comment on attachment 591901 [details] [diff] [review]

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)
if (*s == '\n')
Attached file A test
Unfortunately this test can not be built since MimeHeaders_build_heads_list is not exported...
Comment on attachment 591901 [details] [diff] [review]

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?
Attachment #591901 - Flags: review?(dbienvenu) → review-
Attached patch patch v2Splinter Review
Attachment #591901 - Attachment is obsolete: true
Attachment #592183 - Flags: review?(dbienvenu)
(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 on attachment 592183 [details] [diff] [review]
patch v2

thx for the patch.
Attachment #592183 - Flags: review?(dbienvenu) → review+
Keywords: checkin-needed
Checked in:
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
You need to log in before you can comment on or make changes to this bug.