1731214 - Crash in [@ memcpy_repmovs | nsMsgLineBuffer::BufferInput]

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Wayne Mery (:wsmwk)]

Hard to say whether the increase of the past few months is version 91, or coming out of summer

Crash report: https://crash-stats.mozilla.org/report/index/5796013b-106b-4af5-bff4-1b4ff0210917 91.1.1

Reason: EXCEPTION_ACCESS_VIOLATION_WRITE

Top 10 frames of crashing thread:

0 vcruntime140.dll memcpy_repmovs f:\dd\vctools\crt\vcruntime\src\string\amd64\memcpy.asm:115
1 xul.dll nsMsgLineBuffer::BufferInput comm/mailnews/base/src/nsMsgLineBuffer.cpp:118
2 xul.dll nsMsgMailboxParser::OnDataAvailable comm/mailnews/local/src/nsParseMailbox.cpp:71
3 xul.dll nsMailboxProtocol::ProcessProtocolState comm/mailnews/local/src/nsMailboxProtocol.cpp:664
4 xul.dll nsMsgProtocol::OnDataAvailable comm/mailnews/base/src/nsMsgProtocol.cpp:300
5 xul.dll nsInputStreamPump::OnStateTransfer netwerk/base/nsInputStreamPump.cpp:532
6 xul.dll nsInputStreamPump::OnInputStreamReady netwerk/base/nsInputStreamPump.cpp:377
7 xul.dll nsBufferedInputStream::OnInputStreamReady netwerk/base/nsBufferedStreams.cpp:704
8 xul.dll mozilla::SlicedInputStream::OnInputStreamReady xpcom/io/SlicedInputStream.cpp:416
9 xul.dll nsInputStreamReadyEvent::Run xpcom/io/nsStreamUtils.cpp:94

bp-cacdf7e5-eb36-49e4-96a3-200030210917 version 78

Comment 1

[Magnus Melin [:mkmelin]]

Must be some miscalculations here? https://searchfox.org/comm-central/rev/35c9e2929a5ae37d07192315db3787dc01f73441/mailnews/base/src/nsMsgLineBuffer.cpp#118

Comment 2

[Wayne Mery (:wsmwk)]

#84 crash for 91.2.1 vs #200 for 78.14.0, so the frequency has increased with version 91 (with reference to comment 0)

(In reply to Magnus Melin [:mkmelin] from comment #1)

Must be some miscalculations here? https://searchfox.org/comm-central/rev/35c9e2929a5ae37d07192315db3787dc01f73441/mailnews/base/src/nsMsgLineBuffer.cpp#118

Can we find the error?

FWIW, some of the more recent changes here have been

• Bug 1534124 - Make nsMsgImapLineDownloadCache::SpaceAvailable() more robust
• Bug 1333038 - Crash in nsMsgLineStreamBuffer::ReadNextLine
• Bug 1718721 - nsMsgLineBuffer has a lot of unused code.

Comment 3

[Makoto Kato [:m_kato]]

Interesting point is that all crashes seem to be x64. So as long as I look code, I guess that mailbox is larger than 4GB?

• GrowBuffer should have uint64_t for desired_size parameter, then if it is over 4GB (PR_UINT32_MAX), return NS_ERROR_OUT_OF_MEMORY.
• Use uint64_t desired_size = (end - net_buffer) ...; instead of uint32_t
• (Optional) m_bufferPos and m_bufferSize should be 'size_t'.

Comment 4

[Wayne Mery (:wsmwk)]

Top 45 crash for 91.10.0. Exists in beta 101 bp-b06a4d4b-d557-441f-83be-c46100220518

(In reply to Makoto Kato [:m_kato] from comment #3)

Interesting point is that all crashes seem to be x64. So as long as I look code, I guess that mailbox is larger than 4GB?

• GrowBuffer should have uint64_t for desired_size parameter, then if it is over 4GB (PR_UINT32_MAX), return NS_ERROR_OUT_OF_MEMORY.
• Use uint64_t desired_size = (end - net_buffer) ...; instead of uint32_t
• (Optional) m_bufferPos and m_bufferSize should be 'size_t'.

Sufficent changes to addrewss this crash? Or should we make those changes a seperate bug report?

Mac signature nsMsgLineBuffer::BufferInput bp-e9574524-bd82-42e5-a04a-a40030220611
second Windows signature memcpy | nsMsgLineBuffer::BufferInput bp-207629a2-2d8e-48ca-8395-4d29b0220610

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: Bug 1731214 - Try to fix crash in [@ memcpy_repmovs | nsMsgLineBuffer::BufferInput]. r=kaie

Comment 6

[Magnus Melin [:mkmelin]]

Let's see how it goes.

Comment 7

[Kai Engert (:KaiE:)]

Having a message data stream more than 4 GB big, without any newline, that doesn't seem plausible, right?

Comment 8

[Kai Engert (:KaiE:)]

What's the input for this code?

I assume it's guaranteed to be ASCII data, otherwise it wouldn't make sense to check for newlines.

Is the input an incoming email? That certainly cannot be that big.

Is it possible that the input is a local mailbox file? Then it could be big, yes. But still, a single message without newlines should never be that big.

Could we consider anything as big as 2 GB an internal consistency (possibly a consequence of memory corruption), and refuse to process it at all?

I'll suggest an alternative patch.

Comment 9

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:KaiE:) from comment #8)

I'll suggest an alternative patch.

added review comments in phab

Comment 10

[Magnus Melin [:mkmelin]]

(In reply to Kai Engert (:KaiE:) from comment #8)

What's the input for this code?

For the case triggering it, I don't know. Seems the class is used for mailboxes but also for individual messages.
It seems likely bugs elsewhere would trigger it somehow, and probably we'd end up in some error condition. It's hard to say.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/30ffb4b69a08
Try to fix crash in [@ memcpy_repmovs | nsMsgLineBuffer::BufferInput]. r=kaie

Comment 12

[Kai Engert (:KaiE:)]

Magnus, do you want to request uplift, or should I help?

Comment 13

[Rob Lemley [:rjl]]

Comment on attachment 9280815 [details]
Bug 1731214 - Try to fix crash in [@ memcpy_repmovs | nsMsgLineBuffer::BufferInput]. r=kaie

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Testing completed (on c-c, etc.): On 103.0b1.
Risk to taking this patch (and alternatives if risky):

Comment 14

[Wayne Mery (:wsmwk)]

I'm inclined to have this wait until 102.0.2

Comment 15

[Magnus Melin [:mkmelin]]

Good to get into 102 yes. I wonder if this bug might also be dupe/related to bug 1776633.

Comment 16

[Wayne Mery (:wsmwk)]

Comment on attachment 9280815 [details]
Bug 1731214 - Try to fix crash in [@ memcpy_repmovs | nsMsgLineBuffer::BufferInput]. r=kaie

[Triage Comment]
approved for esr102

Unfortunately we mised this for 102.0.2.

(In reply to Magnus Melin [:mkmelin] from comment #15)

Good to get into 102 yes. I wonder if this bug might also be dupe/related to bug 1776633.

We should know about bug 1776633 when we get this on 102.

Comment 17

[Rob Lemley [:rjl]]

Thunderbird 102.0.3:
https://hg.mozilla.org/releases/comm-esr102/rev/5a9e5f1d8445