Closed Bug 1333038 Opened 3 years ago Closed 2 years ago

Crash in nsMsgLineStreamBuffer::ReadNextLine

Categories

(MailNews Core :: Networking: IMAP, defect)

x86
Windows 7
defect
Not set
critical

Tracking

(thunderbird_esr52 wontfix, thunderbird_esr6064+ fixed, thunderbird63 wontfix, thunderbird64 fixed, thunderbird65 fixed)

RESOLVED FIXED
Thunderbird 65.0
Tracking Status
thunderbird_esr52 --- wontfix
thunderbird_esr60 64+ fixed
thunderbird63 --- wontfix
thunderbird64 --- fixed
thunderbird65 --- fixed

People

(Reporter: richard.leger, Assigned: jorgk-bmo)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-fcba9afc-520d-4ee3-bac7-0599b2170111.
=============================================================
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
Component: Untriaged → Networking: IMAP
Product: Thunderbird → MailNews Core
Depends on: 1264302
See Also: → 1444389
Duplicate of this bug: 1333032
Blocks: 1294074
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] = ' ';
Flags: needinfo?(m_kato)
bp-4fd6c485-027b-4778-aae9-45ac80180406 is another example, from chn.doc
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...
m_kato, bp-bbc2a3cf-0587-40ae-905d-8ea650180531 writes "trying to delete 12 photos"
Keywords: crash
(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.
Flags: needinfo?(m_kato)
See Also: → 1444201
Richard, was it likely a large message or have attachments?
Status: UNCONFIRMED → NEW
Depends on: 1444389
Ever confirmed: true
Flags: needinfo?(richard.leger)
See Also: 1444389
I am not in measure to confirm... it has been two years since original report...
Flags: needinfo?(richard.leger)
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.
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.
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.
This should do it.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #9020657 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9020657 [details] [diff] [review]
1333038-fix-crash.patch

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

Looks great, r=mkmelin
Attachment #9020657 - Flags: review?(mkmelin+mozilla) → review+
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
I forgot to post the try run from last night:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cf332648bd6d6bef3924ed0afcc6171dd96d294b
Target Milestone: --- → Thunderbird 65.0
Attachment #9020657 - Flags: approval-comm-esr60?
Attachment #9020657 - Flags: approval-comm-beta+
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
Flags: needinfo?(vseerror)
Comment on attachment 9020657 [details] [diff] [review]
1333038-fix-crash.patch

Good to go for TB 60.4 ESR.
Attachment #9020657 - Flags: approval-comm-esr60? → approval-comm-esr60+
Depends on: 1516501
Depends on: 1534119
Depends on: 1581017
You need to log in before you can comment on or make changes to this bug.