1333038 - Crash in nsMsgLineStreamBuffer::ReadNextLine

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Richard Leger]

This bug was filed from the Socorro interface and is 
report bp-fcba9afc-520d-4ee3-bac7-0599b2170111.
=============================================================

Comment 1

[Wayne Mery (:wsmwk)]

bp-fcba9afc-520d-4ee3-bac7-0599b2170111

0 	xul.dll	nsMsgLineStreamBuffer::ReadNextLine(nsIInputStream*, unsigned int&, bool&, nsresult*, bool)	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/base/util/nsMsgLineBuffer.cpp:385
1 	xul.dll	nsImapProtocol::CreateNewLineFromSocket()	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapProtocol.cpp:4704
2 	xul.dll	nsImapServerResponseParser::GetNextLineForParser(char**)	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:88
3 	xul.dll	nsIMAPGenericParser::AdvanceToNextLine()	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsIMAPGenericParser.cpp:148
4 	xul.dll	nsImapServerResponseParser::msg_fetch_literal(bool, int)	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:3115
5 	xul.dll	nsImapServerResponseParser::msg_fetch_content(bool, int, char const*)	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:2158
6 	xul.dll	nsImapServerResponseParser::msg_fetch()	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:1349
7 	mozglue.dll	arena_bin_nonfull_run_get	memory/mozjemalloc/jemalloc.c:3968
8 	mozglue.dll	arena_malloc_small	memory/mozjemalloc/jemalloc.c:4241
9 	mozglue.dll	imalloc	memory/mozjemalloc/jemalloc.c:4313
10 	xul.dll	NS_strtok(char const*, char**)	xpcom/glue/nsCRTGlue.cpp:55
11 	xul.dll	nsImapServerResponseParser::ParseIMAPServerResponse(char const*, bool, char*)	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:204

Comment 3

[Wayne Mery (:wsmwk)]

m_kato anything obvious in this 60.0b6 crash?
bp-f4ab4afa-5013-43f4-980a-e5e260180522
0 	xul.dll	nsMsgLineStreamBuffer::ReadNextLine(nsIInputStream*, unsigned int&, bool&, nsresult*, bool)	
1 	xul.dll	nsImapProtocol::CreateNewLineFromSocket()	
2 	xul.dll	nsImapServerResponseParser::GetNextLineForParser(char**)	
3 	xul.dll	nsIMAPGenericParser::AdvanceToNextLine()	
4 	xul.dll	nsIMAPGenericParser::AdvanceToNextToken()	

https://hg.mozilla.org/releases/comm-beta/file/tip/mailnews/imap/src/nsImapProtocol.cpp#l4798 
char* nsImapProtocol::CreateNewLineFromSocket()
{
  bool needMoreData = false;
  char * newLine = nullptr;
  uint32_t numBytesInLine = 0;
  nsresult rv = NS_OK;
  // we hold a ref to the input stream in case we get cancelled from the
  // ui thread, which releases our ref to the input stream, and can
  // cause the pipe to get deleted before the monitor the read is
  // blocked on gets notified. When that happens, the imap thread
  // will stay blocked.
  nsCOMPtr <nsIInputStream> kungFuGrip = m_inputStream;
  do
  {
    newLine = m_inputStreamBuffer->ReadNextLine(m_inputStream, numBytesInLine, needMoreData, &rv);
    MOZ_LOG(IMAP, LogLevel::Debug, ("ReadNextLine [stream=%p nb=%u needmore=%u]\n",
        m_inputStream.get(), numBytesInLine, needMoreData));

  } while (!newLine && NS_SUCCEEDED(rv) && !DeathSignalReceived()); // until we get the next line and haven't been interrupted


https://hg.mozilla.org/releases/comm-beta/annotate/tip/mailnews/base/util/nsMsgLineBuffer.cpp
:382 ends at startOfNewData [i] = ' ';

    uint32_t numBytesToCopy = std::min(uint64_t(numFreeBytesInBuffer - 1) /* leave one for a null terminator */, numBytesInStream);
    if (numBytesToCopy > 0)
    {
      // read the data into the end of our data buffer
      char *startOfNewData = startOfLine + m_numBytesInBuffer;
      rv = aInputStream->Read(startOfNewData, numBytesToCopy, &numBytesCopied);
      if (prv)
        *prv = rv;
          startOfNewData[i] = ' ';

Comment 4

[Wayne Mery (:wsmwk)]

bp-4fd6c485-027b-4778-aae9-45ac80180406 is another example, from chn.doc

Comment 5

[gene smith]

Wayne, In comm-beta and comm-central I see line 382 as:
m_dataBuffer[m_startPos + m_numBytesInBuffer] = '\0';

I haven't seen the crash but I added this right before my line 382 to see if the index is always in range:
MOZ_ASSERT(m_startPos + m_numBytesInBuffer < m_dataBufferSize, "write unalloced mem\n");

There is already a check above that m_dataBuffer is not null. 

Running OK with the new MOZ_ASSERT so far...

Comment 6

[Wayne Mery (:wsmwk)]

m_kato, bp-bbc2a3cf-0587-40ae-905d-8ea650180531 writes "trying to delete 12 photos"

Comment 7

[Makoto Kato [:m_kato]]

(In reply to Wayne Mery (:wsmwk) from comment #6)
> m_kato, bp-bbc2a3cf-0587-40ae-905d-8ea650180531 writes "trying to delete 12
> photos"

This crash might be depends on IMAP data stream and buffer size.  But I don't know about root cause.

Comment 8

[Wayne Mery (:wsmwk)]

Richard, was it likely a large message or have attachments?

Comment 9

[Richard Leger]

I am not in measure to confirm... it has been two years since original report...

Comment 10

[Jorg K (CEST = GMT+2)]

The original report and bp-bbc2a3cf-0587-40ae-905d-8ea650180531 both crash at nsMsgLineBuffer.cpp:385 but that code seems to have shifted since in a recent report bp-b32ff12d-9287-4928-bca1-db5a20181001 it's at like 382 which is:

      m_numBytesInBuffer += numBytesCopied;
382   m_dataBuffer[m_startPos + m_numBytesInBuffer] = '\0';

as Gene already pointed out in comment #5. Hard so say what's going wrong here.

Comment 11

[Wayne Mery (:wsmwk)]

Something has made the crash worse.  Overall crash rate is ~30% higher than 6 months ago.  And it looks like the crash rate of version 60 specifically is 3x-4x higher than version 52.

Comment 12

[Jorg K (CEST = GMT+2)]

I've inspected a few crashes from
https://crash-stats.mozilla.com/signature/?signature=nsMsgLineStreamBuffer%3A%3AReadNextLine
and despite nsMsgLineStreamBuffer::ReadNextLine() being used by all/most protocols, all crashes come from IMAP.

Interesting is the comment before the call:
  // we hold a ref to the input stream in case we get cancelled from the
  // ui thread, which releases our ref to the input stream, and can
  // cause the pipe to get deleted before the monitor the read is
  // blocked on gets notified. When that happens, the imap thread
  // will stay blocked.

So this suggests to me that something gets cancelled, maybe the IMAP protocol is destroyed which deletes m_inputStreamBuffer which will free the buffer which may still be being read on another thread.

So it's not that anything goes out of bounds here, it's that the underlying memory is already being free'ed while someone else is still busily writing into it.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1333038-fix-crash.patch

This should do it.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9020657 [details] [diff] [review]
1333038-fix-crash.patch

Review of attachment 9020657 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, r=mkmelin

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2dd2b075db9e
Use 'modern' pointers to fix crash due to nsMsgLineStreamBuffer object being deleted while still in use. r=mkmelin DONTBUILD

Comment 16

[Jorg K (CEST = GMT+2)]

I forgot to post the try run from last night:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cf332648bd6d6bef3924ed0afcc6171dd96d294b

Comment 17

[Jorg K (CEST = GMT+2)]

Beta (TB 64 beta 2):
https://hg.mozilla.org/releases/comm-beta/rev/3a98ae26df27398c1f868eb016daebd384aeecb2

Comment 18

[Wayne Mery (:wsmwk)]

Another possible signature for this stack.  But we might not know until the patch reaches release 60.4.0

RtlEnterCriticalSection | PR_Lock | PR_EnterMonitor | nsImapProtocol::TellThreadToDie 
bp-55c7dc2a-8a6b-4101-9f21-05a200181101  63.0b1
 0 	ntdll.dll	RtlEnterCriticalSection	
1 	nss3.dll	PR_Lock	nsprpub/pr/src/threads/combined/prulock.c:213
2 	nss3.dll	PR_EnterMonitor	nsprpub/pr/src/threads/prmon.c:131
3 	xul.dll	nsImapProtocol::TellThreadToDie()	comm/mailnews/imap/src/nsImapProtocol.cpp:1282
4 	xul.dll	nsImapProtocol::CreateNewLineFromSocket()	comm/mailnews/imap/src/nsImapProtocol.cpp:4855
5 	xul.dll	nsImapServerResponseParser::GetNextLineForParser(char**)	comm/mailnews/imap/src/nsImapServerResponseParser.cpp:86
6 	xul.dll	nsIMAPGenericParser::AdvanceToNextLine()	comm/mailnews/imap/src/nsIMAPGenericParser.cpp:146
7 	xul.dll	nsImapServerResponseParser::msg_fetch_literal(bool, int)	comm/mailnews/imap/src/nsImapServerResponseParser.cpp:3094
8 	xul.dll	nsImapServerResponseParser::msg_fetch_content(bool, int, char const*)	comm/mailnews/imap/src/nsImapServerResponseParser.cpp:2139
9 	xul.dll	nsImapServerResponseParser::mime_part_data()	comm/mailnews/imap/src/nsImapServerResponseParser.cpp:2664
10 	xul.dll	nsImapServerResponseParser::msg_fetch()	comm/mailnews/imap/src/nsImapServerResponseParser.cpp:1031
11 	xul.dll	nsImapServerResponseParser::response_data()	comm/mailnews/imap/src/nsImapServerResponseParser.cpp:701
12 	xul.dll	nsImapServerResponseParser::ParseIMAPServerResponse(char const*, bool, char*)	comm/mailnews/imap/src/nsImapServerResponseParser.cpp:204
13 	xul.dll	nsImapProtocol::ParseIMAPandCheckForNewMail(char const*, bool)	comm/mailnews/imap/src/nsImapProtocol.cpp:1942

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 9020657 [details] [diff] [review]
1333038-fix-crash.patch

Good to go for TB 60.4 ESR.

Comment 20

[Jorg K (CEST = GMT+2)]

TB 60.4 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/53892f6ffc3a37e0f0c9a4290950f079a68f9425