Closed
Bug 564698
Opened 14 years ago
Closed 14 years ago
crash [@ msg_quote_phrase_or_addr]
Categories
(MailNews Core :: MIME, defect)
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)
719 bytes,
patch
|
Details | Diff | Splinter Review | |
3.22 KB,
patch
|
asuth
:
review+
standard8
:
superreview+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•14 years ago
|
||
tentatively, proactively, looking for 3.1 wanted or blocking
blocking-thunderbird3.1: --- → ?
Component: General → MIME
Product: Thunderbird → MailNews Core
QA Contact: general → mime
Comment 2•14 years ago
|
||
bienvenu, do you have any thoughts as to what's going on here?
Assignee: nobody → bienvenu
Assignee | ||
Comment 3•14 years ago
|
||
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.
Comment 4•14 years ago
|
||
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.
Comment 5•14 years ago
|
||
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.
Comment 6•14 years ago
|
||
(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.
Reporter | ||
Comment 7•14 years ago
|
||
pm'd bp-5f9269ee-0bb1-4639-bc22-169472100509 (ndallas) bp-90a625ce-3924-4fdf-b27c-52b0e2100507 (ich) bp-def2bcc1-9fe8-46a4-b03e-7e0672100506 (huub)
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.
Comment 9•14 years ago
|
||
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.
Assignee | ||
Comment 10•14 years ago
|
||
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.
Assignee | ||
Comment 11•14 years ago
|
||
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.
Comment 12•14 years ago
|
||
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...
Comment 13•14 years ago
|
||
Okay, that same case can be reproduced with a string of ": ;" too.
Comment 14•14 years ago
|
||
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:;"
Assignee | ||
Updated•14 years ago
|
blocking-thunderbird3.1: ? → rc1+
Assignee | ||
Comment 15•14 years ago
|
||
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...
Assignee | ||
Comment 16•14 years ago
|
||
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]
Assignee | ||
Comment 17•14 years ago
|
||
Attachment #444751 -
Flags: superreview?(bugzilla)
Attachment #444751 -
Flags: review?(bugmail)
Assignee | ||
Updated•14 years ago
|
Group: core-security
Assignee | ||
Comment 18•14 years ago
|
||
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.
Assignee | ||
Comment 19•14 years ago
|
||
this was caused by the fix for Bug 242693 so does not affect 3.0.5
Blocks: 242693
Updated•14 years ago
|
status-thunderbird3.0:
--- → unaffected
Comment 20•14 years ago
|
||
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.
Comment 21•14 years ago
|
||
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\"@"
Assignee | ||
Comment 22•14 years ago
|
||
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.
Comment 23•14 years ago
|
||
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 24•14 years ago
|
||
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+
Assignee | ||
Comment 25•14 years ago
|
||
(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!
Assignee | ||
Comment 26•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [has possible fix] → [has fix for review]
Comment 27•14 years ago
|
||
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+
Assignee | ||
Comment 28•14 years ago
|
||
(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 :-(
Comment 29•14 years ago
|
||
(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.
Updated•14 years ago
|
Attachment #445016 -
Flags: superreview?(bugzilla) → superreview+
Assignee | ||
Comment 30•14 years ago
|
||
fixed on trunk and 1.9.2 comm-central branch for 3.1
Status: NEW → RESOLVED
Closed: 14 years ago
status-thunderbird3.1:
--- → rc1-fixed
Resolution: --- → FIXED
Assignee | ||
Comment 31•14 years ago
|
||
thx very much to timeless and asuth for their help
Whiteboard: [has fix for review]
Reporter | ||
Comment 33•14 years ago
|
||
v.fixed according to crash-stats. and huub. 3.1b2 20100430130203 is the last build to show crashes
Status: RESOLVED → VERIFIED
Updated•13 years ago
|
Crash Signature: [@ msg_quote_phrase_or_addr]
Updated•13 years ago
|
Group: core-security
Keywords: regression
You need to log in
before you can comment on or make changes to this bug.
Description
•