crash MimeHeaders_build_heads_list even if bug 624419 is landed

RESOLVED FIXED in Thunderbird 13.0

Status

MailNews Core
MIME
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: m_kato, Assigned: aceman)

Tracking

({crash})

Trunk
Thunderbird 13.0
x86
Windows NT
crash

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

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

Description

6 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/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
(Reporter)

Updated

6 years ago
Component: General → MIME
Product: Thunderbird → MailNews Core
QA Contact: general → mime
(Assignee)

Comment 1

6 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.
(Assignee)

Comment 3

6 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.
(Assignee)

Comment 4

6 years ago
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.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #591901 - Flags: review?(bugmail)
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.
Attachment #591901 - Flags: review?(bugmail) → review?(dbienvenu)
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++;
Created attachment 592028 [details]
A test

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

Comment 8

6 years ago
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?
Attachment #591901 - Flags: review?(dbienvenu) → review-
(Assignee)

Comment 9

6 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

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

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

Updated

6 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/4e8a635051f2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.