Closed Bug 1444389 Opened 2 years ago Closed 2 years ago

Crash in nsImapProtocol::HandleMessageDownLoadLine


(MailNews Core :: Networking: IMAP, defect)

Not set


(thunderbird_esr6063+ fixed, thunderbird63 wontfix, thunderbird64 fixed)

Thunderbird 64.0
Tracking Status
thunderbird_esr60 63+ fixed
thunderbird63 --- wontfix
thunderbird64 --- fixed


(Reporter: wsmwk, Assigned: mkmelin)


(Blocks 1 open bug)


(Keywords: crash, testcase-wanted)

Crash Data


(1 file, 1 obsolete file)

perhaps related to bug 1264302 / bug 1216951 and friends.
exists in 58 beta and 56 beta.

bp-05147860-e5c7-4940-842e-ac06c0180308 59.0b2

Top 10 frames of crashing thread:

0 xul.dll nsImapProtocol::HandleMessageDownLoadLine C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapProtocol.cpp:3995
1 xul.dll nsIMAPBodypart::GenerateBoundary C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsIMAPBodyShell.cpp:484
2 xul.dll nsIMAPBodypartMultipart::Generate C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsIMAPBodyShell.cpp:988
3 xul.dll nsIMAPBodypartMultipart::Generate C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsIMAPBodyShell.cpp:985
4 xul.dll nsIMAPBodypartMultipart::Generate C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsIMAPBodyShell.cpp:985
5 xul.dll nsIMAPBodypartMessage::Generate C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsIMAPBodyShell.cpp:842
6 xul.dll nsIMAPBodyShell::Generate C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsIMAPBodyShell.cpp:263
7 xul.dll nsImapServerResponseParser::ProcessOkCommand C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:422
8 xul.dll nsImapServerResponseParser::ParseIMAPServerResponse C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:255
9 xul.dll nsImapProtocol::ParseIMAPandCheckForNewMail C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapProtocol.cpp:1944

Looking at the beta source, it's crashing on the first line quoted here:
  if (((m_downloadLineCache->CurrentUID() != GetServerStateParser().CurrentResponseUID()) && !m_downloadLineCache->CacheEmpty()) ||
      (m_downloadLineCache->SpaceAvailable() < lineLength + 1) )

No idea, perhaps 'm_downloadLineCache' is null here? A reproducible case would help.
Jorg, thanks.  I will try to get testcase
Flags: needinfo?(vseerror)
Keywords: testcase-wanted
See Also: → 1333038
(In reply to Wayne Mery (:wsmwk) from comment #2)
> Jorg, thanks.  I will try to get testcase

Nothing actionable so far, unfortunately
bp-4d8842b8-3cf7-4cc1-9d25-59dd30180928 is crashing at 

uint32_t lineLength = strlen(messageLine);

That would make messageLine null? The precondition/assertion doesn't do anything on non-debug builds.
I don't expect to find a testcase
Flags: needinfo?(vseerror)
(In reply to Magnus Melin [:mkmelin] from comment #4)
> bp-4d8842b8-3cf7-4cc1-9d25-59dd30180928 is crashing at
> cpp#3850 
> uint32_t lineLength = strlen(messageLine);
> That would make messageLine null? The precondition/assertion doesn't do
> anything on non-debug builds.
Flags: needinfo?(jorgk)
Blocks: 1333038
See Also: 1333038
What's the question?

The code is:
  const char *messageLine = line;
  uint32_t lineLength = strlen(messageLine);
and 'line' is passed in. As Magnus said, something is fishy with argument checking in that function. Magnus, would you like to propose a patch?
Flags: needinfo?(jorgk) → needinfo?(mkmelin+mozilla)
Attached patch bug1444389_line_null_crash.patch (obsolete) — Splinter Review
I suppose this would be it.
Checked the callers and didn't find any that didn't check. There's a few that pass in a member variable, so maybe it's one of those who will mess around with the member inbetween the check and the calling of this.
Assignee: nobody → mkmelin+mozilla
Flags: needinfo?(mkmelin+mozilla)
Attachment #9013262 - Flags: review?(jorgk)
Comment on attachment 9013262 [details] [diff] [review]

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

Well, this looks a bit like a band-aid, but I don't have a better idea. That interface is truly terrible, but sadly the three parameter variant is called twice in nsImapServerResponseParser.cpp :-( - So not easy to remove.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +3842,5 @@
>  //   ensured *before* invoking this method).
>  void nsImapProtocol::HandleMessageDownLoadLine(const char *line, bool isPartialLine,
>                                                 char *lineCopy)
>  {
> +  if (line == nullptr)

Do you prefer this, or !line?

Or maybe more modern:
MOZ_ASSERT(line, "line must not be null");
if (!line)


if (NS_WARN_IF(line == nullptr))

@@ +3844,5 @@
>                                                 char *lineCopy)
>  {
> +  if (line == nullptr)
> +  {
> +    NS_WARNING("line must be set");

I'd go for the option above. What does "be set" mean?
Attachment #9013262 - Flags: review?(jorgk) → review+
I have someone who crashes a lot, but we don't have a clear testcase nor steps. Hopefully he will be willing to test a patched build.
That is, in addition to Richard ... if he chooses to help test
Looks like the official thing to do is

Since we don't have a return value, I used NS_ENSURE_TRUE_VOID().
Attachment #9013262 - Attachment is obsolete: true
Attachment #9015082 - Flags: review+
Attachment #9015082 - Flags: feedback?(mkmelin+mozilla)
Attachment #9015082 - Flags: feedback?(mkmelin+mozilla) → feedback+
Comment on attachment 9015082 [details] [diff] [review]
bug1444389_line_null_crash.patch (JK)

[Triage Comment]
Attachment #9015082 - Flags: approval-comm-esr60?
Attachment #9015082 - Flags: approval-comm-beta+
Pushed by
Fix crash in nsImapProtocol::HandleMessageDownLoadLine(). r=jorgk
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
Comment on attachment 9015082 [details] [diff] [review]
bug1444389_line_null_crash.patch (JK)

[Triage Comment]
Attachment #9015082 - Flags: approval-comm-esr60? → approval-comm-esr60+
Blocks: 1534119
You need to log in before you can comment on or make changes to this bug.