1625685 - crash @ strlen | nsTSubstring<T>::Append

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

+++ This bug was initially created as a clone of Bug #1609690 +++

Crashing at same rate for 68.5.0 and 68.6.0 despite patch landing in 68.4.2 from Bug #1609690

Different cause perhaps ?

bp-f41fae07-e095-4e94-9a83-295930200328 68.6.0
0 ucrtbase.dll strlen
1 xul.dll nsTSubstring<char>::Append(char const*, unsigned int, std::nothrow_t const&) xpcom/string/nsTSubstring.cpp:765
2 xul.dll nsTSubstring<char>::Append(char const*, unsigned int) xpcom/string/nsTSubstring.cpp:754
3 xul.dll nsDelAttachListener::StartProcessing(nsMessenger*, nsIMsgWindow*, nsAttachmentState*, bool) comm/mailnews/base/src/nsMessenger.cpp:2606
4 xul.dll nsMessenger::DetachAttachments(unsigned int, char const**, char const**, char const**, char const**, nsTArray<nsTString<char> >, bool) comm/mailnews/base/src/nsMessenger.cpp:2744
5 xul.dll nsMessenger::DetachAllAttachments(unsigned int, char const**, char const**, char const**, char const**, bool, bool) comm/mailnews/base/src/nsMessenger.cpp:2664
6 xul.dll NS_InvokeByIndex
7 xul.dll XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) js/xpconnect/src/XPCWrappedNative.cpp:1158
8 xul.dll XPC_WN_CallMethod(JSContext, unsigned int, JS::Value*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:946
9 xul.dll js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:535
10 xul.dll static bool InternalCall(struct JSContext*, const class js::AnyInvokeArgs& const) js/src/vm/Interpreter.cpp:590
11 xul.dll static bool Interpret(struct JSContext*, class js::RunState& const) js/src/vm/Interpreter.cpp:3082
12 xul.dll js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:423
13 xul.dll js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:563
14 xul.dll static bool InternalCall(struct JSContext*, const class js::AnyInvokeArgs& const) js/src/vm/Interpreter.cpp:590
15 xul.dll js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:606

Comment 1

[Magnus Melin [:mkmelin]]

On trunk this is at https://searchfox.org/comm-central/rev/36361fb3e76ab8f11bc59524f444269ff6ae84d0/mailnews/base/src/nsMessenger.cpp#2511

Comment 3

[Wayne Mery (:wsmwk)]

Ben, can you resolve this?

With Brendel's excellent description from bug 1639235 I was easily able to reproduce this on beta bp-0605ee2e-1b77-446e-a537-1ffcf0200920

"I receive often emails (especially from Appel users) with many non-usefull attachments. When I

1. first detach one or two usefull attachments, and
2. then use "delete all" to clean up the rest.

I still get the window that warns me which attachments will be deteled , I cilck OK, and then Thunderbird crashes."

Magnus: "Crash is from: https://searchfox.org/comm-central/rev/cfa832291ae06d6b814f8a9ebe1991a08f25da92/mailnews/base/src/nsMessenger.cpp#2513 and from there the -1 case so we end up at https://searchfox.org/mozilla-central/rev/61fceb7c0729773f544a9656f474e36cd636e5ea/xpcom/string/nsTSubstring.cpp#727 which crash."

Comment 4

[Wayne Mery (:wsmwk)]

Another potential https://crash-stats.mozilla.org/signature/?product=Thunderbird&signature=OOM%20%7C%20large%20%7C%20NS_ABORT_OOM%20%7C%20nsTSubstring%3CT%3E%3A%3AAppend&version=78.2.2&date=%3C2020-09-20T20%3A07%3A26%2B00%3A00&date=%3E%3D2020-09-13T20%3A07%3A26%2B00%3A00

Comment 5

[Wayne Mery (:wsmwk)]

Another example, but perhaps a different bug?, is bp-be6ddf9c-be08-49ab-bdcd-ae4030200920 from https://support.mozilla.org/en-US/questions/1305148 The user describes it thusly "It will load, partially check the emails and then crash."

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to Wayne Mery (:wsmwk) from comment #5)

Another example, but perhaps a different bug?, is bp-be6ddf9c-be08-49ab-bdcd-ae4030200920 from https://support.mozilla.org/en-US/questions/1305148 The user describes it thusly "It will load, partially check the emails and then crash."

All of this user's crashes are startup. Stack is significantly different from comment 0
0 ucrtbase.dll strlen
1 xul.dll nsTSubstring<char>::Append(char const*, unsigned int, std::nothrow_t const&) xpcom/string/nsTSubstring.cpp:727
2 xul.dll nsTSubstring<char>::Append(char const*, unsigned int) xpcom/string/nsTSubstring.cpp:716
3 xul.dll nsMailtoUrl::ParseMailtoUrl(char*) comm/mailnews/compose/src/nsSmtpUrl.cpp:0
4 xul.dll nsMailtoUrl::ParseUrl() comm/mailnews/compose/src/nsSmtpUrl.cpp:271
5 xul.dll nsMailtoUrl::SetSpecInternal(nsTSubstring<char> const&) comm/mailnews/compose/src/nsSmtpUrl.cpp:236
6 xul.dll nsMailtoUrl::Mutator::SetSpec(nsTSubstring<char> const&, nsIURIMutator**) comm/mailnews/compose/src/nsSmtpUrl.h:63
7 xul.dll static nsSmtpService::NewMailtoURI(nsTSubstring<char> const&, char const*, nsIURI*, nsIURI**) comm/mailnews/compose/src/nsSmtpService.cpp:276
8 xul.dll NS_NewMailnewsURI(nsIURI**, nsTSubstring<char> const&, char const*, nsIURI*) comm/mailnews/base/src/nsNewMailnewsURI.cpp:51
9 xul.dll NS_NewURI(nsIURI**, nsTSubstring<char> const&, char const*, nsIURI*) netwerk/base/nsNetUtil.cpp:1893
10 xul.dll NS_NewURI(nsIURI**, nsTSubstring<char> const&, mozilla::NotNull<const mozilla::Encoding*>, nsIURI*) netwerk/base/nsNetUtil.cpp:1648
11 xul.dll NS_NewURI(nsIURI**, nsTSubstring<char16_t> const&, mozilla::NotNull<const mozilla::Encoding*>, nsIURI*) netwerk/base/nsNetUtil.cpp:1668
12 xul.dll nsGenericHTMLElement::GetURIAttr(nsAtom*, nsAtom*, nsIURI**) const dom/html/nsGenericHTMLElement.cpp:1504
13 xul.dll nsGenericHTMLElement::GetHrefURIForAnchors() const dom/html/nsGenericHTMLElement.cpp:558
14 xul.dll mozilla::dom::HTMLAnchorElement::GetHrefURI() const dom/html/HTMLAnchorElement.cpp:227
15 xul.dll mozilla::dom::Link::LinkState() const dom/base/Link.cpp:135
16 xul.dll mozilla::dom::Document::FlushPendingLinkUpdates() dom/base/Document.cpp:12011
17 xul.dll mozilla::detail::RunnableMethodImpl<(anonymous namespace)::HangMonitorChild*, void ((anonymous namespace)::HangMonitorChild::*)(), 0, mozilla::RunnableKind::Standard>::Run() xpcom/threads/nsThreadUtils.h:1237

Comment 7

[gene smith]

Attachment: detach.diff

Not sure why I am CC'd on this bug but the STR sounded easy. The diff shows a fix that seems to work, I think.
But the strange thing is that I first saw the MOZ_ASSERTs fail before a crash. I first then replaced the MOZ_ASSERT with printf to see why the 4 lengths didn't match and then I see the real crash. But as printf's they all match! First I put in the "printf("gds 1...) and they all matched and the other ASSERT occurred. I put in the "printf("gds: 2...)" and they matched too! I also put in "printf("gds: 3...)" and it never occurred.

So with printf's the diff works but without the printf's and calling MOZ_ASSERT() the assert hits before the actual crash caused by null partId.
This is with 12 attachments with one detached and saved. The crash (or assert) occur when you deleted all the remaining attachments as described in STR. FWIW, the printf's show all 4 lengths as 12.

Comment 8

[gene smith]

Attachment: detach2.diff

Looks like those MOZ_ASSERT(a==b==c==d) always assert when all 4 are the same non-boolean equal values due to left to right evaluation and conversion to boolean. So you have to do MOZ_ASSERT( a==b && b==c && c==d).

Anyhow this seems to be working but with my 12 attachment message I end up with 3 copies of the email after doing the STR:

1. The original with all 12

2. One with attachment-1 gone (detached and saved to file) and 11 still there.

3. One with attachment-1 gone (detached and saved file) and 11 with placeholder mimes saying the attachment is deleted.

Not sure if multiples copies like this are expected or not.
Edit: Problem is due to charter/spectrum's Openwave server. See next comment.

Comment 9

[gene smith]

Attachment: detach3.diff

Sorry, attached the wrong file. Previous comment applies.
Edit: I tried it on another imap server account and didn't get the extra copies. This seemed familiar and I found where on my Openwave server I noticed this problem before. See bug 1487851 comment 27.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9176798 [details] [diff] [review]
detach3.diff

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

::: mailnews/base/src/nsMessenger.cpp
@@ +2524,5 @@
>        sHeader.Append(',');
>        if (detaching) detachToHeader.Append(',');
>      }
>      partId = GetAttachmentPartId(mAttach->mAttachmentArray[u].mUrl.get());
> +    if (partId) 

These are "internal" urls so not sure why they would ever be wrong. But since they seem to be....

I'd just use
NS_ENSURE_TRUE(partId, NS_ERROR_UNEXPECTED);

@@ +2530,2 @@
>      nextField = PL_strchr(partId, '&');
>      sHeader.Append(partId, nextField ? nextField - partId : -1);

and check nextField. 
We should not ever send Append the -1 which will just crash. 

Maybe
if (!nextField) {
     nextField = PL_strlen(partId) + 1;
   }
sHeader.Append(partId, nextField - partId);


... would work, but didn't try it out

Comment 11

[Wayne Mery (:wsmwk)]

https://support.mozilla.org/en-US/questions/1305148 also commented "The first time, the crashes started with me trying to add an extra dictionary. The application crashed in the process and never recovered. But the second time I did not add anything. Uninstalled everything, removed the profiles... and still the same."

Comment 12

[gene smith]

Attachment: detach4.diff

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

Comment on attachment 9176798 [details] [diff] [review]
detach3.diff

Review of attachment 9176798 [details] [diff] [review]:

::: mailnews/base/src/nsMessenger.cpp
@@ +2524,5 @@

   sHeader.Append(',');
   if (detaching) detachToHeader.Append(',');
 }
 partId = GetAttachmentPartId(mAttach->mAttachmentArray[u].mUrl.get());

• if (partId)

These are "internal" urls so not sure why they would ever be wrong. But
since they seem to be....

Once you have detached an attachment which saves it to your selected place in the filesystem and the you choose "delete all" attachments, the url for the detached attachment is now a url to the file system like: file:///home/gene/Pictures/0002%20rs.jpg.

The attempt to extract the part ID (like 1.2 or 1.3) from this URL fails because there is no part= to be found in the string resulting in a null value for pointer partId.

This show the outputs of added printfs when cycling through the loop while deleting the remaining attachments. Only the first URL is to the file system. The other items were never detached so their URLs are still imap form containing "part" and "filename" separated with '&' as expected. So skipping the url when partId is null fixes the crash (caused by null pointer passed into Append).

gds: mUrl=file:///home/gene/Pictures/0002%20rs.jpg
gds: partId false                                 // skips the Append() called
gds: u=0, sHeader=attach&del=, detachToHeader=&detachTo=

gds: mUrl=imap://[e-mail]:143/fetch%3EUID%3E.INBOX.gds%3E29?part=1.3&filename=0003.JPG
gds: partId=1.3&filename=0003.JPG
gds: nextField=&filename=0003.JPG
gds: u=1, sHeader=attach&del=,1.3, detachToHeader=&detachTo=

gds: mUrl=imap://[e-mail]:143/fetch%3EUID%3E.INBOX.gds%3E29?part=1.4&filename=0004%20rs.jpg
gds: partId=1.4&filename=0004%20rs.jpg
gds: nextField=&filename=0004%20rs.jpg
gds: u=2, sHeader=attach&del=,1.3,1.4, detachToHeader=&detachTo=

gds: mUrl=imap://[e-mail]:143/fetch%3EUID%3E.INBOX.gds%3E29?part=1.5&filename=0005.JPG
gds: partId=1.5&filename=0005.JPG
gds: nextField=&filename=0005.JPG
gds: u=3, sHeader=attach&del=,1.3,1.4,1.5, detachToHeader=&detachTo=
:

I'd just use
NS_ENSURE_TRUE(partId, NS_ERROR_UNEXPECTED);

This prevents the "delete all" attachments from working at all if any have already been detached.

@@ +2530,2 @@

 nextField = PL_strchr(partId, '&');
 sHeader.Append(partId, nextField ? nextField - partId : -1);

and check nextField.
We should not ever send Append the -1 which will just crash.

The -1 is actually a special value that tells Append() to treat the append call as sHeader.Append(partId) so the whole length of partId is appended. I don't see this used much in the codebase but it is documented here: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Glue_classes/nsACString/Append
and the code is here:
https://searchfox.org/comm-central/rev/ecb867f734791071f843a6b0117c394cd0e3d720/mozilla/xpcom/string/nsTSubstring.cpp#749

I'm not sure the -1 is portable between various 32/64 bit platforms but I think if the value PR_UINT32_MAX is used instead of -1 it is. I stepped into Append() with gdb and verified that it does see PR_UINT32_MAX as size_type(-1). (However, in my testing I haven't found a real world case where nextField is null causing -1 to be passed into Append).

Maybe
if (!nextField) {
nextField = PL_strlen(partId) + 1;
}
sHeader.Append(partId, nextField - partId);

... would work, but didn't try it out

I don't think that will work. nextField is a char* and not an int. Maybe this:

if (!nextField) {
      nextField = partId + PL_strlen(partId) + 1;
 }
sHeader.Append(partId, nextField - partId);

However, I don't see a problem with Append() call as is.

Attached is a diff of my changes that fix the crash and that show all the printfs corresponding the output fragment above. I'll attach a cleaned up patch next.

Comment 13

[gene smith]

Attachment: 1625685-avoid-crash-after-delete-all-attachments-with-some-detached.patch

Here's the clean patch w/o printfs for review. Same functionality as detach4.diff.
This also includes fixes at three MOZ_ASSERTS() that were "crashing" only with debug builds when doing the STR for this bug and were masking the actual crash (see comment 8).

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9177044 [details] [diff] [review]
1625685-avoid-crash-after-delete-all-attachments-with-some-detached.patch

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

::: mailnews/base/src/nsMessenger.cpp
@@ +2522,5 @@
>      }
>      partId = GetAttachmentPartId(mAttach->mAttachmentArray[u].mUrl.get());
> +    if (partId) {
> +      nextField = PL_strchr(partId, '&');
> +      sHeader.Append(partId, nextField ? nextField - partId : PR_UINT32_MAX);

-1 should be portable, so let's leave that part as is instead of PR_UINT32_MAX.
Thanks for the fix!

Comment 15

[gene smith]

Attachment: 1625685-avoid-crash-after-delete-all-attachments-with-some-detached-v2.patch

-1 should be portable, so let's leave that part as is instead of PR_UINT32_MAX.

I think you're right. Changed back to -1.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c1249f910fed
Avoid crash when deleting ALL attachments with one or more previously detached. r=mkmelin

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b2e5e3e9cc79
followup - clang format. rs=clang-format

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9177489 [details] [diff] [review]
1625685-avoid-crash-after-delete-all-attachments-with-some-detached-v2.patch

[Approval Request Comment]
Low risk crash fix.

Comment 19

[Wayne Mery (:wsmwk)]

Comment on attachment 9177489 [details] [diff] [review]
1625685-avoid-crash-after-delete-all-attachments-with-some-detached-v2.patch

[Triage Comment]
Approved for beta

Comment 20

[Rob Lemley [:rjl]]

Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/79b1e6ad28a1
https://hg.mozilla.org/releases/comm-beta/rev/5a38f179b694

Comment 21

[Wayne Mery (:wsmwk)]

Comment on attachment 9177489 [details] [diff] [review]
1625685-avoid-crash-after-delete-all-attachments-with-some-detached-v2.patch

[Triage Comment]
Approved for esr78

Comment 22

[Rob Lemley [:rjl]]

Thunderbird 78.4.0:
https://hg.mozilla.org/releases/comm-esr78/rev/4424369108bf
https://hg.mozilla.org/releases/comm-esr78/rev/831319f8ffbc

Comment 23

[Wayne Mery (:wsmwk)]

_platform_strlen | nsTSubstring<T>::Append is gone in 78.4.0 and newer versions

But oddly, strlen | nsTSubstring<T>::Append was not gone in 78.4.0. But does not exist for 78.4.1 and later. I can't explain why.