Crash in nsMsgLineStreamBuffer::ReadNextLine

RESOLVED FIXED in Thunderbird 65.0

Status

defect
--
critical
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: richard.leger, Assigned: jorgk)

Tracking

(Depends on 1 bug, Blocks 1 bug, {crash})

unspecified
Thunderbird 65.0
x86
Windows 7
Dependency tree / graph

Thunderbird Tracking Flags

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

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

2 years ago
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

Updated

2 years ago
Component: Untriaged → Networking: IMAP
Product: Thunderbird → MailNews Core

Updated

2 years ago
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

Comment 5

Last year
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)

Updated

11 months ago
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
Reporter

Comment 9

9 months ago
I am not in measure to confirm... it has been two years since original report...
Flags: needinfo?(richard.leger)
Assignee

Comment 10

9 months ago
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.
Assignee

Comment 12

8 months ago
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.
Assignee

Comment 13

8 months ago
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+

Comment 15

8 months ago
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: 8 months ago
Resolution: --- → FIXED
Assignee

Comment 16

8 months ago
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
Assignee

Updated

8 months ago
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)
Assignee

Comment 19

7 months ago
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+

Updated

6 months ago
Depends on: 1516501
Assignee

Updated

6 months ago

Updated

4 months ago
Depends on: 1534119
You need to log in before you can comment on or make changes to this bug.