Crash in nsImapProtocol::HandleMessageDownLoadLine

RESOLVED FIXED in Thunderbird 64.0

Status

defect
--
critical
RESOLVED FIXED
Last year
4 months ago

People

(Reporter: wsmwk, Assigned: mkmelin)

Tracking

(Blocks 1 bug, {crash, testcase-wanted})

Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr6063+ fixed, thunderbird63 wontfix, thunderbird64 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

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) )
    FlushDownloadCache();

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 https://dxr.mozilla.org/comm-esr60/source/mailnews/imap/src/nsImapProtocol.cpp#3850 

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
> https://dxr.mozilla.org/comm-esr60/source/mailnews/imap/src/nsImapProtocol.
> 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)
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
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Attachment #9013262 - Flags: review?(jorgk)
Comment on attachment 9013262 [details] [diff] [review]
bug1444389_line_null_crash.patch

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)
  return;

or

if (NS_WARN_IF(line == nullptr))
  return;

@@ +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
NS_ENSURE_ARG_POINTER(arg) which is NS_ENSURE_TRUE(arg, NS_ERROR_INVALID_POINTER).

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 mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d3a3e12d2654
Fix crash in nsImapProtocol::HandleMessageDownLoadLine(). r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 10 months 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.