Crash in nsMsgLineStreamBuffer::ReadNextLine

RESOLVED FIXED in Thunderbird 65.0

Status

defect
--
critical
RESOLVED FIXED
2 years ago
2 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

Comment 3

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

Comment 4

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

Comment 5

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

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

Updated

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

9 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

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

Comment 10

7 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

6 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

6 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

6 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
Last Resolved: 6 months ago
Resolution: --- → FIXED
(Assignee)

Comment 16

6 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

6 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

5 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

4 months ago
Depends on: 1516501
(Assignee)

Updated

4 months ago

Updated

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