Closed
Bug 389920
Opened 17 years ago
Closed 17 years ago
Segmentation fault sending mail
Categories
(Thunderbird :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ben, Unassigned)
References
()
Details
(Keywords: fixed1.8.1.8)
Attachments
(1 file, 1 obsolete file)
563 bytes,
patch
|
ch.ey
:
review+
Bienvenu
:
superreview+
dveditz
:
approval1.8.1.8+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•17 years ago
|
||
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•17 years ago
|
||
Ben: you need to ask review (and sr) for you patch by setting the "review?"-flag of the attachment to a suitable reviewer, like bienvenu@nventure.com Marking NEW since we got a patch.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Updated•17 years ago
|
Attachment #274262 -
Flags: review?(bienvenu)
Reporter | ||
Comment 3•17 years ago
|
||
OK, "review?" flag has been set to bienvenu@nventure.com
Comment 4•17 years ago
|
||
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.
Attachment #274262 -
Flags: superreview+
Attachment #274262 -
Flags: review?(ch.ey)
Attachment #274262 -
Flags: review?(bienvenu)
Comment 5•17 years ago
|
||
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.
Attachment #274262 -
Flags: review?(ch.ey) → review-
Reporter | ||
Comment 6•17 years ago
|
||
I concur with Christian, to the extent of my limited knowledge of the XPCOM API. His proposal looks better to me.
Reporter | ||
Comment 7•17 years ago
|
||
Updated patch with revisions as proposed by Christian. I have rebuilt and retested thunderbird and confirmed that this patch still fixes the segfault.
Attachment #274262 -
Attachment is obsolete: true
Attachment #274413 -
Flags: superreview?(ch.ey)
Attachment #274413 -
Flags: review?(bienvenu)
Updated•17 years ago
|
Attachment #274413 -
Flags: superreview?(ch.ey)
Attachment #274413 -
Flags: superreview+
Attachment #274413 -
Flags: review?(ch.ey)
Attachment #274413 -
Flags: review?(bienvenu)
Updated•17 years ago
|
Attachment #274413 -
Flags: review?(ch.ey) → review+
Updated•17 years ago
|
Keywords: checkin-needed
Comment 8•17 years ago
|
||
fixed on trunk, thx, Ben!
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Keywords: checkin-needed
Comment 9•17 years ago
|
||
Comment on attachment 274413 [details] [diff] [review] Revised patch for thunderbird to handle empty SMTP continuations simple fix for stability.
Attachment #274413 -
Flags: approval1.8.1.7?
Comment 10•17 years ago
|
||
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
Attachment #274413 -
Flags: approval1.8.1.7? → approval1.8.1.7+
You need to log in
before you can comment on or make changes to this bug.
Description
•