1848264 - Crash renaming folder in [@ strlen | nsTSubstring<T>::Append | nsTSubstring<T>::Append | nsImapProtocol::RenameHierarchyByHand]

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect, P2)

Description

[Wayne Mery (:wsmwk)]

Bp-3b39a795-4e45-48a6-9ea3-dd3590230807 nsCharTraits<T>::length reporter writes "Every time a folder is renamed, I get unsubscribed from almost all folders and have to resubscribe"

Crash report: https://crash-stats.mozilla.org/report/index/54c4aa4d-0b0c-46e2-8e8b-d4a270230526

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  ucrtbase.dll  strlen  
1  xul.dll  nsTSubstring<char>::Append  xpcom/string/nsTSubstring.cpp:786
2  xul.dll  nsTSubstring<char>::Append  xpcom/string/nsTSubstring.cpp:770
3  xul.dll  nsImapProtocol::RenameHierarchyByHand  mailnews/imap/src/nsImapProtocol.cpp:7072
4  xul.dll  nsImapProtocol::OnMoveFolderHierarchy  mailnews/imap/src/nsImapProtocol.cpp:7324
5  xul.dll  nsImapProtocol::ProcessAuthenticatedStateURL  mailnews/imap/src/nsImapProtocol.cpp:8198
6  xul.dll  nsImapProtocol::ProcessCurrentURL  mailnews/imap/src/nsImapProtocol.cpp:1947
7  xul.dll  nsImapProtocol::ImapThreadMainLoop  mailnews/imap/src/nsImapProtocol.cpp:1488
8  xul.dll  nsImapProtocol::RunImapThreadMainLoop  mailnews/imap/src/nsImapProtocol.cpp:1164
9  xul.dll  nsImapProtocolMainLoopRunnable::Run  mailnews/imap/src/nsImapProtocol.cpp:461

Comment 1

[Magnus Melin [:mkmelin]]

Crashes at https://searchfox.org/comm-central/rev/42868cf48486327a375c16e5d11c3d09b800300d/mailnews/imap/src/nsImapProtocol.cpp#6874

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: Bug 1848264 - Fix Crash renaming folder in [@ strlen | nsTSubstring<T>::Append | nsTSubstring<T>::Append | nsImapProtocol::RenameHierarchyByHand]. r=BenC

I think currentName could have ended up as null

Comment 3

[Ben Campbell]

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

I think currentName could have ended up as null

I'm not so sure that's what's happening...

The area where it's crashing (src) looks like:

      // calculate the new name and do the rename
      nsCString newChildName(newParentMailboxName);
      newChildName += (currentName + PL_strlen(oldParentMailboxName)); // KABOOM!

Where currentName and oldParentMailboxName are raw char*.

According to the crash report, it's crashing when reading from 0x000001b4bdaad000. Which is a long way from null...
So I think there's something else going on :-(

I'm a bit busy on other stuff right now, but set me an NI if you'd like me to take a deeper look at it when things are quieter!

Comment 4

[Ben Campbell]

Just some notes, while they are fresh in my head:

m_deletableChildren is used in three places:

nsImapProtocol::RenameHierarchyByHand() and nsImapProtocol::DeleteSubFolders()
At start of each function:

m_deletableChildren = new nsTArray<char*>();

At end of each function they do:

    delete m_deletableChildren;
    m_deletableChildren = nullptr;

So the lifetime of m_deleteableChildren is tied to the scope of those functions.
But m_deleteableChildren can't just be converted to a local variable.
Both of these functions read and remove items from m_deleteableChildren, but they don't add items.

nsImapProtocol::DiscoverMailboxSpec() is the only place where items are added to m_deleteableChildren.

At the very least, I think it'd make sense to at least convert m_deleteableChildren to hold nsCString instead of char*, and be confident that it'd never hold null values.

Not sure it'd solve this bug.... but it might :-)

Comment 5

[Magnus Melin [:mkmelin]]

Thanks, I'll do that!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/623818834410
Fix Crash renaming folder in [@ strlen | nsTSubstring<T>::Append | nsTSubstring<T>::Append | nsImapProtocol::RenameHierarchyByHand]. r=BenC

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9348810 [details]
Bug 1848264 - Fix Crash renaming folder in [@ strlen | nsTSubstring<T>::Append | nsTSubstring<T>::Append | nsImapProtocol::RenameHierarchyByHand]. r=BenC

[Approval Request Comment]
Possible crash fix.

Comment 8

[gene smith]

From comment 0:

Bp-3b39a795-4e45-48a6-9ea3-dd3590230807 nsCharTraits<T>::length reporter writes "Every time a folder is renamed, I get unsubscribed from almost all folders and have to resubscribe"

Was this a one-time crash on 102? I only see one report with this signature.
I reverted the patch and had no problem renaming folders or deleting folder trees. Nothing crashed or became unsubscribed.

It might be better to just back out this patch rather than re-doing the rename and delete folder code with modern string functions and introduce more problems. (See comment 11)

Comment 9

[Wayne Mery (:wsmwk)]

There are very few people crashing (or at least very few reporting crashes) for all of these signatures.

• [strlen | nsTSubstring<T>::Append | nsTSubstring<T>::Append | nsImapProtocol::RenameHierarchyByHand}( https://crash-stats.mozilla.org/signature/?signature=strlen%20%7C%20nsTSubstring%3CT%3E%3A%3AAppend%20%7C%20nsTSubstring%3CT%3E%3A%3AAppend%20%7C%20nsImapProtocol%3A%3ARenameHierarchyByHand&date=%3E%3D2023-03-04T21%3A46%3A00.000Z&date=%3C2023-09-04T21%3A46%3A00.000Z) is only a couple users.
• strlen | nsCharTraits<T>::length is another example - might be only one user

Comment 10

[Wayne Mery (:wsmwk)]

(In reply to gene smith from comment #8)

...
It might be better to just back out this patch rather than re-doing the rename and delete folder code with modern string functions and introduce more problems.

I don't have a strong opinion. But if we have questions and we keep the patch, it should be on beta for a long period before uplift to 115. If we uplift it at all?

Comment 11

[gene smith]

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

(In reply to gene smith from comment #8)

...
It might be better to just back out this patch rather than re-doing the rename and delete folder code with modern string functions and introduce more problems.

I don't have a strong opinion. But if we have questions and we keep the patch, it should be on beta for a long period before uplift to 115. If we uplift it at all?

We made a small tweak to the patch to fix the new regression crash over at bug 1851293. Otherwise, looking closer at the patch attached on this bug 1848264, it appears OK to me. (But it's not clear to me how it fixes the crash(s) that is the subject of this bug.) So my comment 8 is probably moot.

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9348810 [details]
Bug 1848264 - Fix Crash renaming folder in [@ strlen | nsTSubstring<T>::Append | nsTSubstring<T>::Append | nsImapProtocol::RenameHierarchyByHand]. r=BenC

[Triage Comment]
Approved for esr115

Comment 13

[Wayne Mery (:wsmwk)]

Comment on attachment 9348810 [details]
Bug 1848264 - Fix Crash renaming folder in [@ strlen | nsTSubstring<T>::Append | nsTSubstring<T>::Append | nsImapProtocol::RenameHierarchyByHand]. r=BenC

retracting approval for 115 until we can also take the regression patch in bug 1851293

Comment 14

[Wayne Mery (:wsmwk)]

Crash rate is so low that we have no beta crashes, so impossible to infer whether this will fix esr crashes.

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9348810 [details]
Bug 1848264 - Fix Crash renaming folder in [@ strlen | nsTSubstring<T>::Append | nsTSubstring<T>::Append | nsImapProtocol::RenameHierarchyByHand]. r=BenC

[Triage Comment]
Approved for esr115

Comment 16

[Daniel Darnell [:dandarnell]]

Thunderbird 115.2.3:
https://hg.mozilla.org/releases/comm-esr115/rev/45d3f88e6f8b