593829 - crash [@ memcpy | nsBufferedOutputStream::Write(char const*, unsigned int, unsigned int*)] | [@ nsMsgSaveAsListener::OnDataAvailable]

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ memcpy | nsBufferedOutputStream::Write(char const*, unsigned int, unsigned int*)] during compact
99% of crashes are 3.1.  3.0 crashes mention compact, and have a different stack
#84 crash for v3.1.2 

bp-8ff7f6a9-f105-4b4a-99fd-25ad72100905 3.1.2
0	mozcrt19.dll	memcpy	 memcpy.asm:188
1	thunderbird.exe	nsBufferedOutputStream::Write	netwerk/base/src/nsBufferedStreams.cpp:532
2	thunderbird.exe	nsMsgSaveAsListener::OnDataAvailable	mailnews/base/util/nsMsgMailNewsUrl.cpp:895
3	thunderbird.exe	nsImapCacheStreamListener::OnDataAvailable	mailnews/imap/src/nsImapProtocol.cpp:8555
4	thunderbird.exe	nsInputStreamPump::OnStateTransfer	netwerk/base/src/nsInputStreamPump.cpp:510
5	thunderbird.exe	nsInputStreamPump::OnInputStreamReady
6	xpcom_core.dll	nsInputStreamReadyEvent::Run	xpcom/io/nsStreamUtils.cpp:112
7	xpcom_core.dll	nsThread::ProcessNextEvent	xpcom/threads/nsThread.cpp:527
8	xpcom_core.dll	NS_ProcessNextEvent_P	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:250 

less common variation
bp-e10bd336-f729-4a42-a1d3-992692100902
0	mozcrt19.dll	memcpy	 memcpy.asm:188
1	thunderbird.exe	nsBufferedOutputStream::Write	netwerk/base/src/nsBufferedStreams.cpp:532
2	thunderbird.exe	nsMsgSaveAsListener::OnDataAvailable	mailnews/base/util/nsMsgMailNewsUrl.cpp:895
3	thunderbird.exe	nsStreamListenerTee::OnDataAvailable	netwerk/base/src/nsStreamListenerTee.cpp:108
4	xpcom_core.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
5	xpcom_core.dll	nsProxyObjectCallInfo::Run	xpcom/proxy/src/nsProxyEvent.cpp:181
6	xpcom_core.dll	nsThread::ProcessNextEvent	xpcom/threads/nsThread.cpp:527
7	xpcom_core.dll	NS_ProcessNextEvent_P	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:250
8	thunderbird.exe	nsXULWindow::ShowModal	xpfe/appshell/src/nsXULWindow.cpp:416 


bp-7a832cd7-e659-4b9c-b57f-641a72100726 3.0.6 during compact
0	mozcrt19.dll	memcpy	 memcpy.asm:188
1	thunderbird.exe	netwerk/base/src/nsBufferedStreams.cpp:532
2	thunderbird.exe	nsFolderCompactState::OnDataAvailable	mailnews/base/src/nsMsgFolderCompactor.cpp:862
3	thunderbird.exe	nsMailboxProtocol::ReadMessageResponse	mailnews/local/src/nsMailboxProtocol.cpp:586
4	thunderbird.exe	nsMailboxProtocol::ProcessProtocolState	mailnews/local/src/nsMailboxProtocol.cpp:683
5	thunderbird.exe	nsMsgProtocol::OnDataAvailable	mailnews/base/util/nsMsgProtocol.cpp:359
6	thunderbird.exe	nsInputStreamPump::OnStateTransfer	netwerk/base/src/nsInputStreamPump.cpp:508

Comment 1

[Wayne Mery (:wsmwk)]

timeless, does this look easy?

TB5 #64 crash

bp-a204f173-84e0-42fc-8504-61a702110716 (rduff) unabble to forward saved T-Bird e-mail messages from the saved location
EXCEPTION_ACCESS_VIOLATION_READ
0x7600000
0	mozcrt19.dll	memcpy	memcpy.asm:188
1	xul.dll	nsBufferedOutputStream::Write	netwerk/base/src/nsBufferedStreams.cpp:582
2	xul.dll	nsMsgSaveAsListener::OnDataAvailable	mailnews/base/util/nsMsgMailNewsUrl.cpp:896
3	xul.dll	nsImapCacheStreamListener::OnDataAvailable	mailnews/imap/src/nsImapProtocol.cpp:8694
4	xul.dll	nsInputStreamPump::OnStateTransfer	netwerk/base/src/nsInputStreamPump.cpp:510

Comment 2

[David :Bienvenu]

Attachment: possible fix

My theory is that start is passing end, making the count we try to write negative, and crashing. I think this could happen if the prev buffer ends with a <CR> and the next buffer only has <LF> w/o CR. In fact, that could happen if buffer is just the last char of message, and the message ends up with CRLF. This could happen if the message was SAVE_BUF_SIZE + 1 bytes long.

Comment 3

[[e-mail]]

Comment on attachment 548490 [details] [diff] [review]
possible fix

I'm not sure what this is trying to achieve. Would it be better to check for the CRLF before looking for the end of the next line?

Comment 4

[David :Bienvenu]

(In reply to comment #3)
> I'm not sure what this is trying to achieve.

I'm trying to prevent start from advancing past end. This will happen if we have an 8193 byte message, ending with a CRLF. The prevLastChar will be <CR>. We'll look for the next <CR>, won't find it, then we'll look for the next <LF>, which we will find, and point end at. Then, we'll try to advance start to skip the current EOL, which takes it one past end. I can't think of other cases where this will happen, but they could exist.

> Would it be better to check for
> the CRLF before looking for the end of the next line?

I'm a little scared to mess with the control flow in this code significantly. I could preflight the check that catches the above scenario and set start and/or end to null in that case, which would avoid going through the loop one more time. That's probably the better thing to do, because I think we'd end with an extra line ending with the previous patch.

Comment 5

[[e-mail]]

(In reply to comment #4)
> (In reply to comment #3)
> > I'm not sure what this is trying to achieve.
> I'm trying to prevent start from advancing past end. This will happen if we
> have an 8193 byte message, ending with a CRLF. The prevLastChar will be
> <CR>. We'll look for the next <CR>, won't find it, then we'll look for the
> next <LF>, which we will find, and point end at. Then, we'll try to advance
> start to skip the current EOL, which takes it one past end. I can't think of
> other cases where this will happen, but they could exist.
> 
> > Would it be better to check for
> > the CRLF before looking for the end of the next line?
> 
> I'm a little scared to mess with the control flow in this code
> significantly. I could preflight the check that catches the above scenario
> and set start and/or end to null in that case, which would avoid going
> through the loop one more time. That's probably the better thing to do,
> because I think we'd end with an extra line ending with the previous patch.

Ah, so I guess we've been lucky previously whenever there's been a CRLF crossing a read boundary, since we look for, and presumably find, the next CR, and therefore it's safe to advance start.

I still think that, if it's pointing at the LF following a previous CR, we should increment start before looking for the end of the line. In the 8193 byte case, this means that start will now point at the null and we'll bail out.

Comment 6

[David :Bienvenu]

(In reply to comment #5)

> I still think that, if it's pointing at the LF following a previous CR, we
> should increment start before looking for the end of the line. In the 8193
> byte case, this means that start will now point at the null and we'll bail
> out.

I think the 8193 case was a gross simplification on my part. We're getting called in OnDataAvailable, which makes it impossible to reason about how much data is left that we don't know about (and means this bug could be quite a bit more frequent). In theory, after getting a prev block ending in <CR>, we could get a new block that starts with "<LF>some partial line without ending" and then next time in get the "CRLF followed by more lines". I'm leaning towards just checking that start < end, i.e.,

          if (end > start && m_outputStream &&
              PL_strncasecmp(start, "X-Mozilla-Status:", 17) &&

cuz then I think we'll do the right thing in the scenarios I can think of.  But your suggestion might work just as well. I'll give it a try.

Comment 7

[David :Bienvenu]

Attachment: fix per Neil's comment

If this is what you were suggesting (simply moving the check), I think it would fix the problem.

Comment 8

[David :Bienvenu]

fixed on trunk http://hg.mozilla.org/comm-central/rev/9c3800484d74

Comment 9

[David :Bienvenu]

Comment on attachment 548927 [details] [diff] [review]
fix per Neil's comment

safe fix for long-standing crash

Comment 10

[David :Bienvenu]

fix landed for aurora/tb 7 - http://hg.mozilla.org/releases/comm-aurora/rev/c9f70dd77f65