Closed Bug 564698 Opened 14 years ago Closed 14 years ago

crash [@ msg_quote_phrase_or_addr]

Categories

(MailNews Core :: MIME, defect)

x86
Windows Vista
defect
Not set
critical

Tracking

(blocking-thunderbird3.1 rc1+, thunderbird3.1 rc1-fixed, thunderbird3.0 unaffected)

VERIFIED FIXED
Tracking Status
blocking-thunderbird3.1 --- rc1+
thunderbird3.1 --- rc1-fixed
thunderbird3.0 --- unaffected

People

(Reporter: wsmwk, Assigned: Bienvenu)

References

Details

(Keywords: crash, regression, topcrash)

Crash Data

Attachments

(2 files, 1 obsolete file)

crash [@ msg_quote_phrase_or_addr]
topcrash #14 currently for 3.1b2 (not a topcrash for 3.0.x, but also not a regression because of crashes on old builds)
xref bug 554581 memcpy | msg_parse_Header_addresses] and [@ msg_parse_Header_addresses

on this quick pass (1/2 hour of work) I did not find any crashes with comments or email addresses)

bp-99174992-6b44-4be5-90f3-5a8692100320 3.1b1 (and 3.1b2pre and 3.2a1pre 20100403032058)
0	thunderbird.exe	msg_quote_phrase_or_addr	 mailnews/mime/src/nsMsgHeaderParser.cpp:1016
1	thunderbird.exe	msg_parse_Header_addresses	mailnews/mime/src/nsMsgHeaderParser.cpp:818
2	thunderbird.exe	nsMsgHeaderParser::ParseHeaderAddresses	mailnews/mime/src/nsMsgHeaderParser.cpp:196
3	thunderbird.exe	nsMsgHeaderParser::ParseHeadersWithArray	mailnews/mime/src/nsMsgHeaderParser.cpp:186
4	xpcom_core.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
5	thunderbird.exe	XPCWrappedNative::CallMethod	js/src/xpconnect/src/xpcwrappednative.cpp:2721
6	thunderbird.exe	XPC_WN_CallMethod	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1740
7	js3250.dll	js_Invoke	js/src/jsinterp.cpp:1360
8	js3250.dll	js_Interpret	js/src/jsops.cpp:2240
9	js3250.dll	SendToGenerator	js/src/jsiter.cpp:865
10	js3250.dll	generator_op	js/src/jsiter.cpp:978
11	js3250.dll	generator_send	js/src/jsiter.cpp:987
12	js3250.dll	js_Interpret	js/src/jsops.cpp:2208
13	js3250.dll	SendToGenerator	js/src/jsiter.cpp:865
14	js3250.dll	generator_op	js/src/jsiter.cpp:978
15	js3250.dll	generator_send	js/src/jsiter.cpp:987
16	js3250.dll	js_Interpret	js/src/jsops.cpp:2208
17	js3250.dll	js_Invoke	js/src/jsinterp.cpp:1368
18	js3250.dll	js_fun_call	js/src/jsfun.cpp:1955
19	js3250.dll	js_Interpret	js/src/jsops.cpp:2208
20	js3250.dll	js_Invoke	js/src/jsinterp.cpp:1368
21	thunderbird.exe	nsXPCWrappedJSClass::CallMethod	js/src/xpconnect/src/xpcwrappedjsclass.cpp:1696 

slight variation in line# of top frame bp-80075590-3701-4c62-afc7-980ba2100319 3.1b1
0	thunderbird.exe	msg_quote_phrase_or_addr	 mailnews/mime/src/nsMsgHeaderParser.cpp:1010
1	thunderbird.exe	msg_parse_Header_addresses	mailnews/mime/src/nsMsgHeaderParser.cpp:818
2	thunderbird.exe	nsMsgHeaderParser::ParseHeaderAddresses	mailnews/mime/src/nsMsgHeaderParser.cpp:196
3	thunderbird.exe	nsMsgHeaderParser::ParseHeadersWithArray	mailnews/mime/src/nsMsgHeaderParser.cpp:186 

---
variation, line numbers of top 2 frames
bp-9396d5a2-b3ef-499a-895e-723b92100406 v3.0.4
0	thunderbird.exe	msg_quote_phrase_or_addr	 mailnews/mime/src/nsMsgHeaderParser.cpp:808
1	thunderbird.exe	msg_parse_Header_addresses	mailnews/mime/src/nsMsgHeaderParser.cpp:742
2	thunderbird.exe	nsMsgHeaderParser::ParseHeaderAddresses	mailnews/mime/src/nsMsgHeaderParser.cpp:196 

variation, line number of top 2 frames
bp-149d6eda-668d-413e-b93d-6334e2100325

---
---

is the following related crash? or same?  ...
memcpy | msg_quote_phrase_or_addr, which is #4 topcrash for 3.1b2 

most are like bp-7ebb614d-8e71-492f-bf12-d09362100506
0	mozcrt19.dll	memcpy	 memcpy.asm:350
1	thunderbird.exe	msg_quote_phrase_or_addr	mailnews/mime/src/nsMsgHeaderParser.cpp:1039
2	thunderbird.exe	msg_parse_Header_addresses	mailnews/mime/src/nsMsgHeaderParser.cpp:821
3	thunderbird.exe	nsMsgHeaderParser::ParseHeaderAddresses	mailnews/mime/src/nsMsgHeaderParser.cpp:197
4	thunderbird.exe	nsMsgHeaderParser::ParseHeadersWithArray	mailnews/mime/src/nsMsgHeaderParser.cpp:187 

but like msg_quote_phrase_or_addr some line# variations exist, the smallest being like bp-9a3a7707-8c5e-4f8e-a804-74a872100404 
0	mozcrt19.dll	memcpy	 memcpy.asm:350
1	thunderbird.exe	msg_quote_phrase_or_addr	mailnews/mime/src/nsMsgHeaderParser.cpp:1036
2	thunderbird.exe	msg_parse_Header_addresses	mailnews/mime/src/nsMsgHeaderParser.cpp:818 

bigger line# difference
bp-46a3cc8c-7c06-41de-800d-a79e22100506
0	mozcrt19.dll	memcpy	 memcpy.asm:289
1	thunderbird.exe	msg_quote_phrase_or_addr	mailnews/mime/src/nsMsgHeaderParser.cpp:1051
2	thunderbird.exe	msg_parse_Header_addresses	mailnews/mime/src/nsMsgHeaderParser.cpp:821
3	thunderbird.exe	nsMsgHeaderParser::ParseHeaderAddresses	mailnews/mime/src/nsMsgHeaderParser.cpp:197
tentatively, proactively, looking for 3.1 wanted or blocking
blocking-thunderbird3.1: --- → ?
Component: General → MIME
Product: Thunderbird → MailNews Core
QA Contact: general → mime
bienvenu, do you have any thoughts as to what's going on here?
Assignee: nobody → bienvenu
The two main possibilities are that the output buffer for memcpy is not large enough, or the length passed in is negative. If new_length is negative, the PR_Malloc would fail, and I don't see how it could be negative anyway.

The size of the out buffer is guaranteed to be at least (2 * length) + 2

    /* Add 2 to the length for the quotes, plus one for each character
     * which will need a backslash, plus one for a null terminator.
     */
    new_length = length + quotable_count + unquotable_count + 3;

but new_length seems like it could conceivably be (2 * length) + 3, but I don't know if all the characters could be either quotable or unquotable (which I think really should be called needsQuoting and alreadyQuoted)

msg_parse_Header_addresses parses multiple addresses into the same buffer, and allocates (2 * length) + 10. So in theory, if we had > 5 addresses in the original line, the math could get a bit off, if we maxed out the special chars.

Or maybe there's some existing heap corruption we're running into. Seems less likely, though.
This looks like a very promising crash.  All of the exception addresses start on new 4k page boundaries and the code is frankly just terrifying.
Oh yeah, very promising.  It seems like the bulk of the top crashes that are not pure memory allocation things (garbage collection) turn out to be involved with nsMsgHeaderParser.  For example, arena_run_reg_alloc is this dude too.

Sadly, it would appear that the crash reports are not entirely reliable... Most of the crashes cite "mailnews/mime/src/nsMsgHeaderParser.cpp:187" and then have a number of stack frames inside that.  The problem is that line 187 is:
  PR_FREEIF(addresses);

This means either:
1) the debug info is very wrong for mapping the PC back to the line...
2) breakpad had to resort to a stack scan to get us a backtrace and is erroneously reconstructing frames that had already been returned from

Unfortunately, unless the 'raw dump' tab is actually not hiding useful information from me, I'm not sure I will ever know the answer to my question via the crash reports, and they don't have a lot of other love to give.

The easiest thing is probably just to fuzz the heck out of the stupid routine since it looks like we can do so using javascript (gloda appears to be feeding it the death data, so having XPConnect/JS sanitizing us out of the problem space is apparently not an issue) and it's probably faster than trying to play C code detective.
(In reply to comment #5)
> Unfortunately, unless the 'raw dump' tab is actually not hiding useful
> information from me, I'm not sure I will ever know the answer to my question
> via the crash reports, and they don't have a lot of other love to give.

Er that is, "actually hiding useful information".  In other words, if there's a way to get more data from the crash reports than those tabs on the web interface show, someone please speak up and tell me.
5f9269ee-0bb1-4639-bc22-169472100509 is hiding a .dmp file. You need to be a member of the security group or similar to have access to it.
my crash happened directly after the upgrade from a2 to b1, never had it crashing before, however i did delete a bunch of emails from various mail accounts and folders before the upgrade (did not do the actual deletion with thunderbird, if thats relevant, just moved it to trash)
i never had it crashing after this.
The code is unspeakable. It could be rewritten to use nsCString's, along with SetCapacity to avoid extra allocations. But we might still want to figure out what kind of addresses are causing the crashes for test cases.
this crash bp-5f9269ee-0bb1-4639-bc22-169472100509 implies that this calculation is wrong:

new_length = length + quotable_count + unquotable_count + 3;

because we're crashing writing to the locally allocated buffer. Or, we might be reading off the end of the input address, though that seems less likely.
So, I have valgrind showing that with an input of (crappy fuzzer so far):
" a>; $[!) %@ %  <%$$\"a (\\   @   a $ [$: \"  \\%a>>!\\!; a@ \\.:;"
to parseHeadersWithArray, will read beyond the end of the input on line 459:
  while (*line_temp &&

can end up reading beyond the end of the allocation.  namely...
(gdb) x line_end [per valgrind, this is actually the last valid byte in the alloc]
0x12147d20:	0 '\000'
(gdb) x line_temp
0x12147d21:	0 '\000'

Now, that should probably terminate because we found a null beyond the end of the first null, but if line_temp was not also null, we could begin a line of crime off in the land of memory, I expect.  I'll keep poking for a bit...
Okay, that same case can be reproduced with a string of ": ;" too.
Loading Dump File [5f9269ee-0bb1-4639-bc22-169472100509.dmp]

0:000> .frame f
0f 001bebdc 6d995d30 thunderbird!nsMsgHeaderParser::ParseHeadersWithArray+0x6d [e:\buildbot\win32_build\build\mailnews\mime\src\nsmsgheaderparser.cpp @ 187]
0:000> dt tempString
Local var @ 0x1beb44 Type nsAutoString
   +0x000 mData            : 0x001beb58  "undisclosed-recipients:;"
   +0x004 mLength          : 0x18
   +0x008 mFlags           : 0x10011
   +0x00c mFixedCapacity   : 0x3f
   +0x010 mFixedBuf        : 0x001beb58  "undisclosed-recipients:;"
   +0x014 mStorage         : [64]  "undisclosed-recipients:;"
blocking-thunderbird3.1: ? → rc1+
Andrew, can you try this patch with your valgrind test? I'm not sure the error I found is the same part of the code as what you described, but we're clearly marching off the end in the place that I patched...
not quite sure how to write a test for this - I don't have a crashing case, since it seems to partly depend on what's in uninitialized memory. But I'll dig a bit deeper.
Whiteboard: [has possible fix]
Attached patch proposed fix and test (obsolete) — Splinter Review
Attachment #444751 - Flags: superreview?(bugzilla)
Attachment #444751 - Flags: review?(bugmail)
Group: core-security
marking security sensitive because the test case shows how to turn the memory read overrun into a heap trounce. Standard8, I'll check if this affects 3.0.5 as well.
this was caused by the fix for Bug 242693 so does not affect 3.0.5
Blocks: 242693
it looks like valgrind is happy with the :; change and the fuzzer no longer seems to cause over-reads, but it's angry about an uninitialized read that I had previously mistakenly ignored.

The current example in ParseHeadersWithArray has numAddresses = 4, but on index = 3 I am seeing currentAddress point at uninitialized memory.  'names' has four initialized nul characters followed by the uninitalized pattern.   'addresses' has 3 nul-terminated strings and then the uninitialized pattern.

This could conceivably cause a read off the end of a memory page causing a crash if there are no nul's in the way.

I'll look into this one a bit more and try and get a smaller example.
parser.parseHeadersWithArray("\" \"@a a;@", addresses, names, fullAddresses);

gets us the uninitialized read.  both 'names' strings are empty, we only have one 'addresses' string and it is: 
"\"\" \\\"@a a\"@"
I think that case is a separate issue and I'd like to get this fix checked in so we can look at crash-stats, while I work on the second issue. I suspect it's just the trailing '@' that's causing the problem.
Indeed, 'tis a separate issue and we don't need to tie them other than wanting to never have to see that code again after this bug.  I will caution that in the example I gave I think if I removed anything more from the string that valgrind stopped complaining...
Comment on attachment 444751 [details] [diff] [review]
proposed fix and test

The logic in the surrounding area could be simplified, but this definitely plugs this hole.  The test is delightfully nefarious.
Attachment #444751 - Flags: review?(bugmail) → review+
(In reply to comment #23)
>  I will caution that in
> the example I gave I think if I removed anything more from the string that
> valgrind stopped complaining...
Hmm, some quick tests say you're right. Once more unto the breach!
this fixes both issues (asuth, sorry for making you look at parts of the patch twice).

The fix is basically just to calculate how many bytes we actually wrote instead of (mis)calculating it.
Attachment #444751 - Attachment is obsolete: true
Attachment #445016 - Flags: superreview?(bugzilla)
Attachment #445016 - Flags: review?(bugmail)
Attachment #444751 - Flags: superreview?(bugzilla)
Whiteboard: [has possible fix] → [has fix for review]
Comment on attachment 445016 [details] [diff] [review]
cumulative fix for both issues

It might be advisable to calculate the return value before freeing orig_out.  While the code doesn't currently zero the pointer on free, it's the kind of change that would not be surprising, nor would a static analysis tool being unhappy about looking at the value of the pointer after freeing it.

Thanks for taking on fixing these and not pointing the blame at me ;).  Hopefully when the people with pitchforks come for you they will understand that nothing would get shipped if we rewrote things every time we encountered a sketchy section of code.

(In the process of reviewing I should say I did adjust to the code a bit, but this is the type of thing where the parsing is complex enough that a proper state machine would seem appropriate, regardless of whether the raw pointer slinging is replaced with presumably safer string and iterator class usage.)
Attachment #445016 - Flags: review?(bugmail) → review+
(In reply to comment #27)
> (From update of attachment 445016 [details] [diff] [review])
> It might be advisable to calculate the return value before freeing orig_out. 
> While the code doesn't currently zero the pointer on free, it's the kind of
> change that would not be surprising

But that would break our tests :-)

, nor would a static analysis tool being
> unhappy about looking at the value of the pointer after freeing it.

It felt a bit odd to do that, but I don't think it's actually evil. But I can change it.
> 
> (In the process of reviewing I should say I did adjust to the code a bit, but
> this is the type of thing where the parsing is complex enough that a proper
> state machine would seem appropriate, regardless of whether the raw pointer
> slinging is replaced with presumably safer string and iterator class usage.)

This code so needs to be rewritten, I agree. But not a couple days before code freeze :-(
(In reply to comment #28)
> > unhappy about looking at the value of the pointer after freeing it.
> 
> It felt a bit odd to do that, but I don't think it's actually evil. But I can
> change it.

No need to change it.
Attachment #445016 - Flags: superreview?(bugzilla) → superreview+
fixed on trunk and 1.9.2 comm-central branch for 3.1
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
thx very much to timeless and asuth for their help
Whiteboard: [has fix for review]
v.fixed according to crash-stats.  and huub.
3.1b2 20100430130203 is the last build to show crashes
Status: RESOLVED → VERIFIED
Crash Signature: [@ msg_quote_phrase_or_addr]
Group: core-security
Keywords: regression
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: