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

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Tomasz Ostrowski]

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

When opening e-mail with broken vcard that looks like this:
==========================================================================
begin:vcard
fn;quoted-printable:Xxxx=C5=82xx  Xxx
n;quoted-printable:Xxx;Xxxx=C5=82xx
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 2.0.0.4 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 2.0.0.4 on Windows and Linux and Seamonkey 1.1.2 on Windows - all affected.

I think this could be used as some kind of DOS.

Comment 1

[Tomasz Ostrowski]

Attachment: A message triggering this bug

Comment 2

[David :Bienvenu]

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

Comment 3

[David :Bienvenu]

Attachment: possible fix

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".

Comment 4

[David :Bienvenu]

fixed on trunk - requesting 1.8.1.5 approval, after some bake time on the trunk.

Comment 5

[Daniel Veditz [:dveditz]]

Does this affect Thunderbird 1.5.0.12 as well?

Comment 6

[David :Bienvenu]

good question, yes, from looking at the code, I think it does (i.e., it's not a regression)

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 271530 [details] [diff] [review]
possible fix

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

Please land within ~30 hours or it'll have to go next time.

Comment 8

[David :Bienvenu]

fix landed on 1.8.1.5 branch

Comment 9

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.5 using the test email message from comment #1 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.5pre) Gecko/20070711 Thunderbird/2.0.0.5pre ID:2007071104 on Fedora F7 and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.5pre) Gecko/20070710 Thunderbird/2.0.0.5pre Mnenhy/0.7.5.0 ID:2007071100 - Thunderbird don`t crash or eat all memory. Thunderbird stays here with 44 MB Memory Usage.

-> adding verified keyword

Comment 10

[Daniel Veditz [:dveditz]]

This still needs landing on the 1.8.0 branch for an eventual Thunderbird 1.5.0.13

Comment 11

[Kai Engert [:KaiE:]]

Checked in to 1.8.0 branch

Comment 12

[Carsten Book [:Tomcat]]

verified fixed Mozilla/5.0 (Windows; U; Windows NT 5.1; zh-CN; rv:1.8.0.13) Gecko/20070809 Thunderbird/1.5.0.13 ID:2007080918

Thunderbird does not crash and also not eat all memory

Comment 13

[Wayne Mery (:wsmwk)]

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.

Comment 14

[Mark Banner (:standard8)]

Attachment: Testcase

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).

Comment 15

[Mark Banner (:standard8)]

Attachment: [checked in] Testcase v2

Sorry, here's the correct version.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 367858 [details] [diff] [review]
[checked in] Testcase v2

Checked in testcase: http://hg.mozilla.org/comm-central/rev/5cb4e78cb271

Comment 17

[ISHIKAWA, Chiaki]

Hi,

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.")
to
comm/mailnews/addrbook/test/unit/test_bug387403.js.

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