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
bienvenu, do you have any thoughts as to what's going on here?
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.
(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.
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.
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 :  "undisclosed-recipients:;"
Created attachment 444694 [details] [diff] [review] fix reading past end of buffer 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.
Created attachment 444751 [details] [diff] [review] proposed fix and test
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.
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.
(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!
Created attachment 445016 [details] [diff] [review] cumulative fix for both issues 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.
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.)
(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.
fixed on trunk and 1.9.2 comm-central branch for 3.1
thx very much to timeless and asuth for their help
v.fixed according to crash-stats. and huub. 3.1b2 20100430130203 is the last build to show crashes