389920 - Segmentation fault sending mail

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Ben Caradoc-Davies]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-GB; rv:1.8.1.5) Gecko/20070718 Fedora/2.0.0.5-1.fc7 Firefox/2.0.0.5
Build Identifier: Thunderbird 2.0.0.5 x86_64

Originally reported at https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=248243

Problem started after my ISP upgraded their SMTP servers, possibly changing the replies during an SMTP session. Looks like this has revealed a logic error in nsSmtpProtocol::SmtpResponse in mailnews/compose/src/nsSmtpProtocol.cpp . 

Stack trace and gdb session available at:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=248243

Versions known to be affected are all x86_64:

thunderbird-1.5.0.12-1.fc6.x86_64
thunderbird-2.0.0.4-1.fc7.x86_64 
thunderbird-2.0.0.5-1.fc7.x86_64

thunderbird-2.0.0.4-1.fc7.i386.rpm is known to be NOT affected. The workaround is to use an i386 build.

The difference between i386 and x86_64 might be a timing or buffer size issue.

This is a niche error, possibly affecting only the combination of x86_64 and my ISP. High severity, but low priority.  :-)

Analysis:

From my comment in https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=248243

******

Looking in mailnews/compose/src/nsSmtpProtocol.cpp suggests an SMTP response of
"250-", so after stripping the response code and continuation, line becomes ""
(assigned to m_responseText), so its length is zero, causing a -1 index and
segfault. I can't see why this should affect x86_64 only. Perhaps just different
timing to i386?

Or is there something bogus in the handling of cont_char?

Here is some more from gdb on the segfaulted thread:

(gdb) print m_responseText
$6 = {<nsCSubstring> = {<nsACString_internal> = {mVTable = 0x2aaaab22c0d0, 
      mData = 0x20ec6c8 "", mLength = 0, 
      mFlags = 5}, <No data fields>}, <No data fields>}
(gdb) print m_responseCode
$7 = 250
(gdb) print cont_char
$8 = 45 '-'
(gdb) print line
$9 = <value optimized out>
(gdb) print m_continuationResponse
$10 = 250

******


Reproducible: Always

Steps to Reproduce:
1. Compose message
2. Press send button

Actual Results:  
Sending mail progress dialog appears. Application receives segmentation fault. All application windows close.

Expected Results:  
Mail sent.

Comment 1

[Ben Caradoc-Davies]

Attachment: Patch for thunderbird to handle empty SMTP continuations

This thunderbird patch checks for empty SMTP continuation text (such as one
containing only "250-") and prevents a segfault.

I tested the patch by rebuilding thunderbird from
thunderbird-2.0.0.5-1.fc7.src.rpm with the patch included. The patched version
of thunderbird was able to send mail correctly and otherwise appears to behave
as normal.

I suspect that it is pure luck (differing memory layout) that i386 does not
segfault on this SMTP response.

This patch should be applied to thunderbird 1.5 as well as 2.0 as both are
affected by the segfault.

Comment 2

[Magnus Melin [:mkmelin]]

Ben: you need to ask review (and sr) for you patch by setting the "review?"-flag of the attachment to a suitable reviewer, like [e-mail]

Marking NEW since we got a patch.

Comment 3

[Ben Caradoc-Davies]

OK, "review?" flag has been set to [e-mail]

Comment 4

[David :Bienvenu]

Comment on attachment 274262 [details] [diff] [review]
Patch for thunderbird to handle empty SMTP continuations

looks fine to me - we used to prefer m_responseText.IsEmpty() but now that all strings are flat, it doesn't matter so much.

Comment 5

[Christian Eyrich]

Comment on attachment 274262 [details] [diff] [review]
Patch for thunderbird to handle empty SMTP continuations

Right thing to do.
But I think we should use
  if (m_responseText.IsEmpty() || m_responseText.Last() != '\n')
that would be clearer.

Comment 6

[Ben Caradoc-Davies]

I concur with Christian, to the extent of my limited knowledge of the XPCOM API. His proposal looks better to me.

Comment 7

[Ben Caradoc-Davies]

Attachment: Revised patch for thunderbird to handle empty SMTP continuations

Updated patch with revisions as proposed by Christian. I have rebuilt and retested thunderbird and confirmed that this patch still fixes the segfault.

Comment 8

[David :Bienvenu]

fixed on trunk, thx, Ben!

Comment 9

[David :Bienvenu]

Comment on attachment 274413 [details] [diff] [review]
Revised patch for thunderbird to handle empty SMTP continuations

simple fix for stability.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 274413 [details] [diff] [review]
Revised patch for thunderbird to handle empty SMTP continuations

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 11

[David :Bienvenu]

fixed for 1.8.1.7