1710213 - Crash in [@ mozilla::detail::nsTStringRepr<T>::First | nsParseMailMessageState::ParseHeaders]

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Wayne Mery (:wsmwk)]

topcrash starting in 89 beta, with buildid 20210419235748. And the crash signature exists only in beta. But perhaps there is possibility of antivirus involvement. The average user crashes 10 times.

Crash report: https://crash-stats.mozilla.org/report/index/42b35dd4-6e14-4d80-9c2f-22c4a0210507

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(this->mLength > 0) (|First()| called on an empty string)

Top 10 frames of crashing thread:

0 xul.dll mozilla::detail::nsTStringRepr<char>::First const xpcom/string/nsTSubstring.cpp:968
1 xul.dll nsParseMailMessageState::ParseHeaders comm/mailnews/local/src/nsParseMailbox.cpp:877
2 xul.dll nsParseMailMessageState::ParseFolderLine comm/mailnews/local/src/nsParseMailbox.cpp:615
3 xul.dll nsMsgLineBuffer::BufferInput comm/mailnews/base/src/nsMsgLineBuffer.cpp:145
4 xul.dll nsMsgMailboxParser::OnDataAvailable comm/mailnews/local/src/nsParseMailbox.cpp:71
5 xul.dll nsMailboxProtocol::ProcessProtocolState comm/mailnews/local/src/nsMailboxProtocol.cpp:664
6 xul.dll nsMsgProtocol::OnDataAvailable comm/mailnews/base/src/nsMsgProtocol.cpp:300
7 xul.dll nsInputStreamPump::OnStateTransfer netwerk/base/nsInputStreamPump.cpp:548
8 xul.dll nsInputStreamPump::OnInputStreamReady netwerk/base/nsInputStreamPump.cpp:393
9 xul.dll nsBufferedInputStream::OnInputStreamReady netwerk/base/nsBufferedStreams.cpp:724

The crash ID above and bp-7867489a-97ed-42f1-9da6-b3af30210507 and bp-a9f72c23-48df-485e-adeb-59be70210503 all happen ~120 or ~60 seconds. SOme of these have Sophos installed.

And then there is a set of crashes like bp-e97b160d-39f6-47f0-b5a3-d40920210507 that have spybot installed and uptime is mostly <=20 seconds

Comment 1

[Magnus Melin [:mkmelin]]

An empty check needs to be added before the switch. https://searchfox.org/comm-central/rev/17d44e8d85868479357d7bd9dcd3ca82152ef2b6/mailnews/local/src/nsParseMailbox.cpp#875
But why is it empty...

Comment 2

[gene smith]

Attachment: check-empty.diff

I did some testing on this and a way I found to produce an empty string is for the header line to start with a colon. This causes the start and end pointer for the nsDependentCSubstring constructor to be equal which results in a zero length string. (Seems like it should result in a 1 byte length string, but that doesn't seems to be the case.)
Causing other things like "buf" to be null didn't produce an assert or crash.
I've never seen in real life a header line start with a colon so not 100% sure this is really what the users are seeing.
With my proposed change, if the line starts with colon, the line will be ignored (not parsed) in the switch and the next line, if present, will still be evaluated. Alternatively, I guess the loop could be exited in the same manner as occurs when a colon is not found, i.e.,
if (!colon) break;,
but this may result in the message not appearing.

Comment 3

[José M. Muñoz]

The simpler choice would be to go back to the original non-crashing version:
https://hg.mozilla.org/comm-central/rev/48afdbe161e8#l1.15
So:

// If headerStr is empty due to the colon being at the start of the line, we switch on the null-byte.
switch (headerStr.get()[0]) {

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9221200 [details] [diff] [review]
check-empty.diff

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

Would be good to add test data that has a bare : line in it.

::: mailnews/local/src/nsParseMailbox.cpp
@@ +879,5 @@
> +    // First(), only call it for non-empty strings to set firstChar.
> +    // If the string is empty, just set firstChar to the the colon which will
> +    // be ignored in the switch allowing the next hopefully valid header line
> +    // to be parsed in the loop.
> +    char firstChar;

Maybe just 
char firstChar = !headerStr.IsEmpty() ? headerStr.First() : *colon;

Comment 5

[gene smith]

Attachment: check-empty-v2.diff

I think I originally tried to use "headerStr.get()[0]" and it didn't work. When I compile it now I see this error:
no member named 'get' in 'nsTDependentSubstring<char>'

Here's another possibility. Use the constructor that specifies directly the string length rather than the the start and end pointers.

Comment 6

[José M. Muñoz]

Comment on attachment 9221423 [details] [diff] [review]
check-empty-v2.diff

Nice. I'd go for std::max(end - buf, 1). Sorry my initial suggestion didn't compile. In your suggestion (end - buf) doesn't need parenthesis, or does it?

Comment 7

[gene smith]

(In reply to José M. Muñoz from comment #6)

Comment on attachment 9221423 [details] [diff] [review]
check-empty-v2.diff

Nice. I'd go for std::max(end - buf, 1).

The seems good. I don't think it can but what if buf is g.t. end? Will it return 1?

Sorry my initial suggestion didn't compile. In your suggestion (end - buf) doesn't need parenthesis, or does it?

It gave a warning that - happens before the ? which is OK. But adding the parens got rid of the warning. But moot if I go with the max() thing.

Comment 8

[José M. Muñoz]

Well, max(-55, 1) is still 1 ;-)

Comment 9

[gene smith]

I thought maybe since end and buf are actually pointers and not signed ints that maybe it would do unsigned subtraction resulting in a pos. number much bigger than 1. But I guess not.

Anyhow, I'll wait and see what Magnus says on this and I'll probably also add something to the test file like he requested.

Comment 10

[gene smith]

Attachment: Bug1710213-avoid-assert-on-leading-colon.patch

Went ahead and did a formal patch using the std::max() method. It produces almost the same assembly as my proposed method using ? : construct.

Also, added a line starting with colon to the test file. It causes the unchanged code to crash during the unit test but with this full patch applied the relevant unit tests pass and no crash occurs.

Note: To use the std::max() with a parameter that is the difference between two pointer values you have to either define a new int variable that is the difference and pass it to max() or you have to cast the difference to an int as the max() parameter. Otherwise there is a compile error. I just did a cast.

Comment 11

[José M. Muñoz]

Comment on attachment 9221483 [details] [diff] [review]
Bug1710213-avoid-assert-on-leading-colon.patch

Sorry about the lengthy discussion about what is ultimately syntactic sugar. Looks like we all didn't see the forest for the trees. The max solution is indeed questionable since if buf > end, you may get a large positive number.

You started off with nsDependentCSubstring headerStr(buf, (end - buf) ? end - buf : 1); - but why subtract and not just ask for what we really need: nsDependentCSubstring headerStr(buf, (end > buf) ? end - buf : 1);

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9221483 [details] [diff] [review]
Bug1710213-avoid-assert-on-leading-colon.patch

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

I don't think this is very readable vs just allowing the empty string to be created, and handling an empty string later. Thanks for adding the test case data

Comment 13

[José M. Muñoz]

I find the "substring magic" in https://searchfox.org/mozilla-central/source/xpcom/string/nsTDependentSubstring.h a little confusing. Apparently there are two variants (start, end) and (start, length), but its hard to see what happens if start > end. length is unsigned, so the second variant doesn't have the issue.

So when the choice is:

nsDependentCSubstring headerStr(buf, (end > buf) ? end - buf : 1);

vs.

nsDependentCSubstring headerStr(buf, end);
char firstChar = !headerStr.IsEmpty() ? headerStr.First() : *colon;

I'd go with the former.

Comment 14

[gene smith]

Attachment: Bug1710213-avoid-assert-on-leading-colon-v2.patch

If I read the comments right, there is a slight disagreement on the best way to go here. Anyhow, probably either way is OK so, since Magnus is the official reviewer, I'll resubmit the patch using his suggested method. Let me know if this is OK.

Comment 15

[José M. Muñoz]

Comment on attachment 9221650 [details] [diff] [review]
Bug1710213-avoid-assert-on-leading-colon-v2.patch

I don't find this more readable. In any case, I looked at the code to see whether the case buf > end can happen. It can't. If you want to improve the code, I suggest to remove the unnecessary variable end altogether. Also, please run clang-format on the patch, that will remove the trailing space you have in the comment.

Comment 16

[gene smith]

Attachment: Bug1710213-avoid-assert-on-leading-colon-v3.patch

Got rid of end and removed trailing blank. File OK with clang formatter.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9221678 [details] [diff] [review]
Bug1710213-avoid-assert-on-leading-colon-v3.patch

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

Thanks! r=mkmelin

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dfa65d00a857
Avoid assert crash due to leading colon on message header line. r=mkmelin