crash MimeHeaders_build_heads_list even if bug 624419 is landed

RESOLVED FIXED in Thunderbird 13.0


7 years ago
7 years ago


(Reporter: m_kato, Assigned: aceman)



Thunderbird 13.0
Windows NT

Firefox Tracking Flags

(Not tracked)


(crash signature)


(2 attachments, 1 obsolete attachment)

4.76 KB, text/plain
933 bytes, patch
: review+
Details | Diff | Splinter Review


7 years ago
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


7 years ago
Component: General → MIME
Product: Thunderbird → MailNews Core
QA Contact: general → mime

Comment 1

7 years ago
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.

Comment 3

7 years ago
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

7 years ago
Created attachment 591901 [details] [diff] [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')
Created attachment 592028 [details]
A test

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

Comment 8

7 years ago
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-

Comment 9

7 years ago
Created attachment 592183 [details] [diff] [review]
patch v2
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 11

7 years ago
Comment on attachment 592183 [details] [diff] [review]
patch v2

thx for the patch.
Attachment #592183 - Flags: review?(dbienvenu) → review+


7 years ago
Keywords: checkin-needed
Checked in:
Last Resolved: 7 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.