Closed Bug 387403 Opened 14 years ago Closed 14 years ago

Thunderbird eat memory until crash when opening e-mail with broken vcard


(Thunderbird :: Mail Window Front End, defect)

Not set


(Not tracked)



(Reporter: tometzky, Assigned: Bienvenu)


(Keywords: verified1.8.0.13, verified1.8.1.5, Whiteboard: [sg:low dos])


(3 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/20070603 Fedora/ Firefox/
Build Identifier: (20070604)

When opening e-mail with broken vcard that looks like this:
fn;quoted-printable:Xxxx=C5=82xx  Xxx
adr;quoted-printable;quoted-printable;dom:;;xx. Xxxxxxxxxxxx X;Lubacz=C3=3
Thunderbird eats all available memory.

This is an actual vcard received from another Thunderbird user (personal details changed to X-es). I don't know how this user has managed to create a vcard that broken (a city name ends with a half of a letter "ó" (oacute), it should be "Lubaczów"), but it seems it is somehow possible.

Thunderbird should not crash on an e-mail anyway - does not matter how terribly broken one.

Reproducible: Always

Steps to Reproduce:
1. Open the message I'll attach.
Actual Results:  
Thunderbird eats all available memory, slows a computer to a crawl and then crashes.

Expected Results:  
Message is displayed, possibly with an information that attached vcard is broken.

I've tested Thunderbird on Windows and Linux and Seamonkey 1.1.2 on Windows - all affected.

I think this could be used as some kind of DOS.
yeah, this is a simple DOS - marking security sensitive for now, though the work around is not to click on the message. The bug looks to be in mailnews/addrbook/src/nsvcard.cpp
Group: security
Ever confirmed: true
Attached patch possible fixSplinter Review
this fixes the infinite allocating loop. We won't display any of the v-card because basically the v-card parser seems to ignore the whole thing in case of errors. I'm not sure if this is going to break a v-card where we have an empty field at the end, but I don't think that would be valid anyway, since the vcard should end with "end:vcard".
Assignee: nobody → bienvenu
Attachment #271530 - Flags: superreview?(mscott)
Attachment #271530 - Flags: superreview?(mscott) → superreview+
fixed on trunk - requesting approval, after some bake time on the trunk.
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #271530 - Flags: approval1.8.1.5?
Does this affect Thunderbird as well?
good question, yes, from looking at the code, I think it does (i.e., it's not a regression)
Comment on attachment 271530 [details] [diff] [review]
possible fix

approved for and, a=dveditz for release-drivers.

Please land within ~30 hours or it'll have to go next time.
Attachment #271530 - Flags: approval1.8.1.5?
Attachment #271530 - Flags: approval1.8.1.5+
Attachment #271530 - Flags: approval1.8.0.13+
fix landed on branch
Keywords: fixed1.8.1.5
verified fixed using the test email message from comment #1 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/20070711 Thunderbird/ ID:2007071104 on Fedora F7 and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv: Gecko/20070710 Thunderbird/ Mnenhy/ ID:2007071100 - Thunderbird don`t crash or eat all memory. Thunderbird stays here with 44 MB Memory Usage.

-> adding verified keyword
Whiteboard: [sg:low dos]
This still needs landing on the 1.8.0 branch for an eventual Thunderbird
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13+
Checked in to 1.8.0 branch
Keywords: fixed1.8.0.13
verified fixed Mozilla/5.0 (Windows; U; Windows NT 5.1; zh-CN; rv: Gecko/20070809 Thunderbird/ ID:2007080918

Thunderbird does not crash and also not eat all memory
Group: security
Flags: in-testsuite?
This FIXED bug is flagged with in‑testsuite?   It would be great if assignee or someone else can clear the flag if a test is not appropriate.  And if appropriate, create a test and plus the flag to finish off the bug.
Attached patch Testcase (obsolete) — Splinter Review
Testcase for this bug. With the "possible fix" patch backed out, this test hangs and eats memory.

Note that I've downgraded an assertion in nsAbManager to a warning - if the vcard parsing failed, then it is potentially not us (a lot of vcards come from outside). I didn't want to change the return result as looking at the callers they don't handle an error situation, so just picking up a blank card is best (and I doubt we'd have got this far anyway because we'd be attempting to parse a vcard displayed on a message).
Attachment #367848 - Flags: superreview?(bienvenu)
Attachment #367848 - Flags: review?(bienvenu)
Sorry, here's the correct version.
Attachment #367848 - Attachment is obsolete: true
Attachment #367858 - Flags: superreview?(bienvenu)
Attachment #367858 - Flags: review?(bienvenu)
Attachment #367848 - Flags: superreview?(bienvenu)
Attachment #367848 - Flags: review?(bienvenu)
Attachment #367858 - Flags: superreview?(bienvenu)
Attachment #367858 - Flags: superreview+
Attachment #367858 - Flags: review?(bienvenu)
Attachment #367858 - Flags: review+
Comment on attachment 367858 [details] [diff] [review]
[checked in] Testcase v2

Checked in testcase:
Attachment #367858 - Attachment description: Testcase v2 → [checked in] Testcase v2
Flags: in-testsuite? → in-testsuite+


Thanks to a better checking and reporting in the lower-level code, I see the following error message in xpcshell test log of FULL DEBUG version of C-C TB when I used "--verbose" && "--serialize" (to avoid the mixing of logs from different tests executed by different processes)
coming from this test for about a month or so now.:

 3:43.05 pid:319990 JavaScript error: /NEW-SSD/moz-obj-dir/objdir-tb3/_tests/xpcshell/comm/mailnews/addrbook/test/unit/test_bug387403.js, line 12: uncaught exception: ParserError: Missing parameter value in 'fn;quoted-printable:Xxxx=C5=82xx  Xxx'

For long-term maintenance purposes., it would be insanely great to have the test to print out something like the following on start up:
"This test produces 'ParserError: Missing parameter value.'
This is intentional and can safely be ignored."

Maybe we can add
dump(""This test produces 'ParserError: Missing parameter value.'
This is intentional and can safely be ignored.")

But come to think of it, the lower-level code no longer tries to catch and do something sensible about the ParseError(???).

You need to log in before you can comment on or make changes to this bug.