Last Comment Bug 717121 - crash nsMIMEHeaderParamImpl::DoParameterInternal
: crash nsMIMEHeaderParamImpl::DoParameterInternal
Status: RESOLVED FIXED
[tbird topcrash][inbound][qa-]
: crash, regression, topcrash
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla13
Assigned To: Makoto Kato [:m_kato]
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks: 609667
  Show dependency treegraph
 
Reported: 2012-01-10 17:34 PST by Makoto Kato [:m_kato]
Modified: 2012-03-05 17:14 PST (History)
10 users (show)
m_kato: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
11+
fixed


Attachments
fix (1.73 KB, patch)
2012-02-02 16:57 PST, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
fix v1.1 (1.78 KB, patch)
2012-02-02 17:05 PST, Makoto Kato [:m_kato]
bzbarsky: review+
Details | Diff | Splinter Review
fix v2 (1.79 KB, patch)
2012-02-02 21:01 PST, Makoto Kato [:m_kato]
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Makoto Kato [:m_kato] 2012-01-10 17:34:19 PST
Possible overrun.  When line 278, *str may be null since for-loop exits by *str == null.

272     // Skip forward to the end of this token. 
273     for (; *str && !nsCRT::IsAsciiSpace(*str) && *str != '=' && *str != ';'; str++)
274       ;
275     tokenEnd = str;
276
277     // Skip over whitespace, '=', and whitespace
278     while (nsCRT::IsAsciiSpace(*str)) ++str; 



This bug was filed from the Socorro interface and is 
report bp-3afb8381-e7af-49d4-ba83-3615f2120109 .
============================================================= 
0 	xul.dll 	nsMIMEHeaderParamImpl::DoParameterInternal 	netwerk/mime/nsMIMEHeaderParamImpl.cpp:278
1 	xul.dll 	nsMIMEHeaderParamImpl::GetParameterInternal 	netwerk/mime/nsMIMEHeaderParamImpl.cpp:187
2 	xul.dll 	MimeHeaders_get_parameter 	mailnews/mime/src/mimehdrs.cpp:527
3 	xul.dll 	MimeHeaders_get_name 	mailnews/mime/src/mimehdrs.cpp:738
4 	xul.dll 	MimeObject_write 	mailnews/mime/src/mimei.cpp:1791
5 	xul.dll 	MimeInlineTextPlain_parse_line 	mailnews/mime/src/mimetpla.cpp:477
6 	xul.dll 	MimeInlineText_convert_and_parse_line 	mailnews/mime/src/mimetext.cpp:442
7 	xul.dll 	MimeInlineText_rotate_convert_and_parse_line 	mailnews/mime/src/mimetext.cpp:570
8 	xul.dll 	convert_and_send_buffer 	mailnews/mime/src/mimebuf.cpp:184
9 	xul.dll 	mime_LineBuffer 	mailnews/mime/src/mimebuf.cpp:272
10 	xul.dll 	MimeInlineText_parse_decoded_buffer 	mailnews/mime/src/mimetext.cpp:358
11 	xul.dll 	MimeLeaf_parse_buffer 	mailnews/mime/src/mimeleaf.cpp:184
12 	xul.dll 	MimeMultipart_parse_child_line 	mailnews/mime/src/mimemult.cpp:686
13 	xul.dll 	MimeMultipart_parse_line 	mailnews/mime/src/mimemult.cpp:340
14 	xul.dll 	convert_and_send_buffer 	mailnews/mime/src/mimebuf.cpp:184
15 	xul.dll 	mime_LineBuffer 	mailnews/mime/src/mimebuf.cpp:272
16 	xul.dll 	MimeObject_parse_buffer 	mailnews/mime/src/mimeobj.cpp:275
17 	xul.dll 	MimeMessage_parse_line 	mailnews/mime/src/mimemsg.cpp:233
18 	xul.dll 	convert_and_send_buffer 	mailnews/mime/src/mimebuf.cpp:184
19 	xul.dll 	mime_LineBuffer 	mailnews/mime/src/mimebuf.cpp:272
20 	xul.dll 	MimeObject_parse_buffer 	mailnews/mime/src/mimeobj.cpp:275
21 	xul.dll 	mime_display_stream_write 	mailnews/mime/src/mimemoz2.cpp:1004
22 	xul.dll 	nsStreamConverter::OnDataAvailable 	mailnews/mime/src/nsStreamConverter.cpp:979
23 	xul.dll 	nsImapCacheStreamListener::OnDataAvailable 	mailnews/imap/src/nsImapProtocol.cpp:8633
24 	xul.dll 	nsInputStreamPump::OnStateTransfer 	netwerk/base/src/nsInputStreamPump.cpp:510
25 	xul.dll 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:400
26 	xul.dll 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:114
27 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:631
28 	xul.dll 	NS_ProcessNextEvent_P 	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:245
29 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
30 	xul.dll 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:208
31 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:201
32 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:175
33 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
34 	xul.dll 	nsAppShell::Run 	widget/src/windows/nsAppShell.cpp:257
35 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:228
36 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3551
37 	thunderbird.exe 	do_main 	mail/app/nsMailApp.cpp:143
38 	thunderbird.exe 	NS_internal_main 	mail/app/nsMailApp.cpp:226
39 	thunderbird.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:107
Comment 1 Makoto Kato [:m_kato] 2012-01-10 22:04:15 PST
ah, comment #1 will be invalid.

MimeHeaders_get_name may return string that isn't null teminated.
Comment 3 Makoto Kato [:m_kato] 2012-01-16 05:00:59 PST
(In reply to Wayne Mery (:wsmwk) from comment #2)
> all crashes are version 10 [1] 
> => regression?

I don't know...
Comment 4 :aceman 2012-01-25 09:04:15 PST
(In reply to Makoto Kato from comment #1)
> ah, comment #1 will be invalid.
> 
> MimeHeaders_get_name may return string that isn't null teminated.

Did you mean comment 0?
Comment 5 Makoto Kato [:m_kato] 2012-02-01 17:36:44 PST
(In reply to :aceman from comment #4)
> (In reply to Makoto Kato from comment #1)
> > ah, comment #1 will be invalid.
> > 
> > MimeHeaders_get_name may return string that isn't null teminated.
> 
> Did you mean comment 0?

Ah, yes.

This crash will occur if last character of parameter of Content-Disposition header is "'". if last is "'", nsMIMEHeaderParamImpl::DoParameterInternal will skip next null-terminator.
Comment 6 Wayne Mery (:wsmwk, NI for questions) 2012-02-02 12:39:34 PST
#2 crash for tbird v10.
earliest build with crash is 2011122210. does this help hit the regression?

firefox crash almost zero - only 2, and stack is different bp-dd4f4e30-5c2f-4835-846a-515d52120201
Comment 7 Makoto Kato [:m_kato] 2012-02-02 16:57:40 PST
Created attachment 594021 [details] [diff] [review]
fix

test case is possible crash that depends on memory allocator.
Comment 8 Makoto Kato [:m_kato] 2012-02-02 17:05:21 PST
Created attachment 594023 [details] [diff] [review]
fix v1.1
Comment 9 Boris Zbarsky [:bz] 2012-02-02 18:56:18 PST
Comment on attachment 594023 [details] [diff] [review]
fix v1.1

r=me
Comment 10 Makoto Kato [:m_kato] 2012-02-02 21:01:47 PST
Created attachment 594070 [details] [diff] [review]
fix v2
Comment 11 Makoto Kato [:m_kato] 2012-02-02 21:03:26 PST
Comment on attachment 594070 [details] [diff] [review]
fix v2

bz, sorry.  previous fix has typo.  flag was invalid....
Comment 12 Boris Zbarsky [:bz] 2012-02-02 21:20:30 PST
Comment on attachment 594070 [details] [diff] [review]
fix v2

r=me
Comment 14 Ed Morley [:emorley] 2012-02-03 11:03:46 PST
https://hg.mozilla.org/mozilla-central/rev/f65800b074ac
Comment 15 Mark Banner (:standard8) 2012-02-08 09:51:49 PST
Comment on attachment 594070 [details] [diff] [review]
fix v2

[Approval Request Comment]
Regression caused by (bug #): Probably bug 692574 or bug 703015.
User impact if declined: This exhibits a top-crash for Thunderbird.
Testing completed (on m-c, etc.): Has landed on mozilla-central, will be shipped in TB 10.0.1.
Risk to taking this patch (and alternatives if risky): Adds a check for overruning the string length. Includes a test, so should be low risk.
String changes made by this patch: None

Note: I would also like this to be taken on mozilla-esr10 (no approval flags yet).
Comment 16 Julian Reschke 2012-02-08 10:10:46 PST
I'm not convinced the problem is fully understood.

I tested C-D header field values of both

  attachment; filename="

and

  attachment; filename='

and they do not cause a crash. I also notice that the test case tests a trailing double quote, while comment 5 talks about trailing single quotes.

Do we have a concrete test case that causes that crash?
Comment 17 David :Bienvenu 2012-02-08 12:01:06 PST
Reading off the end of a block of allocated memory isn't guaranteed to crash; you have to have a page fault, which makes it trickier to do. to get the test to fail, you can do tricks like memory mapping a file - we have tests that do this, but I can't find them at the moment.
Comment 18 Makoto Kato [:m_kato] 2012-02-08 16:40:13 PST
(In reply to Julian Reschke from comment #16)
> I'm not convinced the problem is fully understood.
> 
> I tested C-D header field values of both
> 
>   attachment; filename="
> 
> and
> 
>   attachment; filename='
> 
> and they do not cause a crash. I also notice that the test case tests a
> trailing double quote, while comment 5 talks about trailing single quotes.
> 
> Do we have a concrete test case that causes that crash?

This test case is possible crash.  Because this crash depends on memory allocator.  jemalloc allocates aligned size instead of requested size.

It is no way to crash easily.  But this fixes possible overrun.
Comment 19 Alex Keybl [:akeybl] 2012-02-09 13:36:48 PST
Comment on attachment 594070 [details] [diff] [review]
fix v2

[Triage Comment]
This has had time to bake on m-c, and is deemed low risk TB top crasher. I'm adding the tracking flag for ESR, although that'll be changing soon. See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Comment 21 Makoto Kato [:m_kato] 2012-02-15 01:20:14 PST
http://hg.mozilla.org/releases/mozilla-beta/rev/e5ec1b3295b7
Comment 22 Alex Keybl [:akeybl] 2012-02-16 15:36:45 PST
Please land on mozilla-esr10 as soon as possible. For more information on the landing process, please see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Comment 24 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-05 10:42:39 PST
Does this only affect Thunderbird, or is there something QA can do to verify this fix in Firefox?
Comment 25 Makoto Kato [:m_kato] 2012-03-05 17:06:57 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #24)
> Does this only affect Thunderbird, or is there something QA can do to verify
> this fix in Firefox?

bp-9aff268d-cd60-465d-a662-313802120218 is Firefox issue.  But this is no easy way to verify this.  crash test is possible case.

But there is no crash on Thunderbird 10.0.1 and 10.0.2.  so I think that this is fixed.
Comment 26 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-05 17:14:41 PST
Calling this qa- based on comment 25.

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