1954306 - Crash in [@ strlen | nsCharTraits<T>::length] via nsParseMailMessageState::FinalizeHeaders

Status: VERIFIED FIXED

(Thunderbird :: General, defect)

Description

[Wayne Mery (:wsmwk)]

strlen | nsCharTraits<T>::length signature continues after bug 1848264 and bug 1907691.

Comment 1

[Wayne Mery (:wsmwk)]

#4 crash for 136.0

Crash report: https://crash-stats.mozilla.org/report/index/1db2a371-d21f-4f9e-ba62-351d60250315

Reason:

EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames:

0  ucrtbase.dll  strlen
1  xul.dll  nsCharTraits<char>::length(char const*)  xpcom/string/nsCharTraits.h:425
1  xul.dll  nsTSubstring<char>::Assign(char const*, unsigned long long, std::nothrow_t co...  xpcom/string/nsTSubstring.cpp:415
2  xul.dll  nsTSubstring<char>::Assign(char const*, unsigned long long)  xpcom/string/nsTSubstring.cpp:394
3  xul.dll  nsParseMailMessageState::FinalizeHeaders()  mailnews/local/src/nsParseMailbox.cpp:713
4  xul.dll  nsParseMailMessageState::ParseFolderLine(char const*, unsigned int)  mailnews/local/src/nsParseMailbox.cpp:236
5  xul.dll  StoreIndexer::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long lon...  mailnews/local/src/StoreIndexer.cpp:181
6  xul.dll  nsInputStreamPump::OnStateTransfer()  netwerk/base/nsInputStreamPump.cpp:585
7  xul.dll  nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*)  netwerk/base/nsInputStreamPump.cpp:412
8  xul.dll  CallbackHolder::CallbackHolder::<lambda_1>::operator()() const  xpcom/io/nsPipe3.cpp:73

Crash report: https://crash-stats.mozilla.org/report/index/40d4339a-5cc9-4ae2-8c37-7d5740250315 128.8.0

Reason:

EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames:

0  ucrtbase.dll  strlen
1  xul.dll  nsCharTraits<char>::length(char const*)  xpcom/string/nsCharTraits.h:423
1  xul.dll  nsTSubstring<char>::Assign(char const*, unsigned long long, std::nothrow_t co...  xpcom/string/nsTSubstring.cpp:422
2  xul.dll  nsTSubstring<char>::Assign(char const*, unsigned long long)  xpcom/string/nsTSubstring.cpp:401
3  xul.dll  nsParseMailMessageState::FinalizeHeaders()  mailnews/local/src/nsParseMailbox.cpp:1031
4  xul.dll  nsParseMailMessageState::ParseFolderLine(char const*, unsigned int)  mailnews/local/src/nsParseMailbox.cpp:530
5  xul.dll  StoreIndexer::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long lon...  mailnews/local/src/StoreIndexer.cpp:181
6  xul.dll  nsInputStreamPump::OnStateTransfer()  netwerk/base/nsInputStreamPump.cpp:585
7  xul.dll  nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*)  netwerk/base/nsInputStreamPump.cpp:412
8  xul.dll  CallbackHolder::CallbackHolder::<lambda_1>::operator()() const  xpcom/io/nsPipe3.cpp:73

Comment 2

[Hartmut Welpmann [:welpy-cw]]

Apart from the same library function involved in the crash, I don't think this relates in any way to bug 1848264 or bug 1907691. The crash from bug 1954381 however happens in a similar way in nsParseMailMessageState::FinalizeHeaders().

Comment 3

[Hartmut Welpmann [:welpy-cw]]

The crash happens here when a Message-ID: header is encountered that either contains comments or is invalid. So maybe fixing bug 697376 could help.

Comment 4

[Hartmut Welpmann [:welpy-cw]]

Attachment: Bug 1954306 - Make Message-ID handling in nsParseMailMessageState more robust. r=#thunderbird-back-end-reviewers

Comment 5

[Hartmut Welpmann [:welpy-cw]]

(In reply to Hartmut Welpmann [:welpy-cw] from comment #3)

The crash happens here when a Message-ID: header is encountered that either contains comments or is invalid.

… which means that it is not enclosed in angle brackets, contrary to what should almost always be the case (see also RFC 5322). If the id->value is not correctly null-terminated either, strlen keeps looking for 0 until it reaches a memory location it is not supposed to access. While all HeaderData objects should contain null-terminated strings (and debug builds halt execution otherwise), there may still be some obscure bug in nsParseMailMessageState::ParseHeaders() that causes them not to. As long as at least id->length is set correctly, the suggested patch may fix this exact crash.

Comment 6

[Hartmut Welpmann [:welpy-cw]]

Try run

I don't think we'll see any effect until this lands in Beta, so leaving open for now.

Comment 7

[Magnus Melin [:mkmelin]]

Probably still better for tracking to close now, and reopen if it didn't work based on beta data.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/10f9cfba96d3
Make Message-ID handling in nsParseMailMessageState more robust. r=mkmelin

Comment 9

[Wayne Mery (:wsmwk)]

No chance of seeing an effect in nightly crash rate - there are none

But perhaps in beta 138, because the past beta crash rate is about one per day

Comment 10

[Wayne Mery (:wsmwk)]

So far, there are no version beta 138 crashes. So when 138 release hits, we should see no crashes there.

With a crash ranking of only ~150, ordinarily this might not be worth uplifting to esr (especially with being close to 140 shipping). But based on stacks [1] this might tie to other decently ranked crashes, so perhaps worthy of uplifting some time 138 is proven, i.e. after 128.10.0.

[1] nsParseMailMessageState::FinalizeHeaders on stack

• PL_strncpy | PL_strncat | nsParseMailMessageState::GetAggregateHeader bug 1581079
• strlen | nsCharTraits<T>::length bug 1907691
• strnlen bug 1581079
• memcpy | nsCharTraits<T>::copy bug 1954381
• MsgUnhex bug 1907773

Comment 11

[Wayne Mery (:wsmwk)]

At this point perhaps not worth uplifting this and bug 1954306 to esr128, given the modest crash rate on 128 even when combining other crash signatures.

And we can assess the crash rates of comment 10 after esr140 ships