474756 - Importing big Address Books makes Thunderbird crash [@ nsTextAddress::GetField(char const*, int, int, nsCString&, char)]

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[((Gabriel)) Tudor]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ro; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ro; rv:1.9.1b3pre) Gecko/20081204 Thunderbird/3.0b1

When wanting to import some big Adress Books in the CSV (separated by commas) or TXT (separated by tabs), Thunderbird crashes and brings up the error dialogue message box. 

Reproducible: Always

Steps to Reproduce:
1. Export a big Adress Book from Thunderbird in CSV or TXT format
2. Import one of this Adress Books in Thunderbird
3. Now, look what happens: TB crashes and asks you to report the crash to Mozilla


Expected Results:  
The dialogue box with the crash reporting popped up.

Comment 1

[Mark Banner (:standard8)]

How do you define "Big"? How many entries, what are your typical filled in fields, etc?

Can you simulate this by generating a synthetic file?

If you can generate or attach a testcase to this bug it would be a great help in reproducing this problem.

In any case, please submit the crashes and let us know the crash ids: http://kb.mozillazine.org/Breakpad

Comment 2

[((Gabriel)) Tudor]

(In reply to comment #1)
> How do you define "Big"? How many entries, what are your typical filled in
> fields, etc?
> 
> Can you simulate this by generating a synthetic file?
> 
> If you can generate or attach a testcase to this bug it would be a great help
> in reproducing this problem.
> 
> In any case, please submit the crashes and let us know the crash ids:
> http://kb.mozillazine.org/Breakpad

I think there are 800 or more entries (don't know for sure, because Thunderbird doesn't have current numbers for the Address Book entries. 

The filled fields are all except in the Other page where just "Notes" is filled. 

I'm sorry, but I don't know how to explain better the crash, as I'm just an ordinary user... 

However, I can send you a video file (.avi) with all that's happening, if you give me an adress or a ftp site where to send it. 

Good luck with all the work for Thunderbird!

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> > In any case, please submit the crashes and let us know the crash ids:
> > http://kb.mozillazine.org/Breakpad

> However, I can send you a video file (.avi) with all that's happening, if you
> give me an adress or a ftp site where to send it. 

A video is no good. Please read the link above and provide the IDs of the crashes - View the crashes as it says under "Viewing Reports" on that page and then paste some of the recent ids here please.

Comment 4

[((Gabriel)) Tudor]

This is what I get from the crash report: 

Add-ons: {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}:1.0.1,[e-mail]:1.3,[e-mail]:0.16,[e-mail]:0.1.20081211,{972ce4c6-7e08-4474-a285-3208198ce6fd}:2.0
BuildID: 20081204115318
CrashTime: 1232643189
InstallTime: 1229383821
ProductName: Thunderbird
SecondsSinceLastCrash: 63590
StartupTime: 1232643131
Theme: classic/1.0
URL: 
UserID: b9bad1fb-7af3-48a9-a03b-99447ced5916
Vendor: 
Version: 3.0b1

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> This is what I get from the crash report: 

What is the file name of the crash report?

Comment 6

[((Gabriel)) Tudor]

I copied this text from the crash report window that pops up when Thunderbird hangs. It was in a text box after you press Details. 

The rest of the information, I got from clicking on the crash reports IDs from the Error Console after I pressed Evaluate

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> The rest of the information, I got from clicking on the crash reports IDs from
> the Error Console after I pressed Evaluate

Please tell us those ids...

Comment 8

[((Gabriel)) Tudor]

e6c865b0-a6f2-46d0-ab44-8fd492090122

adf46916-ef3a-4988-be46-376ac2090121

61243f9d-1074-4f46-a9d3-4d70f2090121

1ea4fa0e-4863-4ccf-aca2-8deb82090121

f4fbd522-0435-4396-b2f1-bcaf72081216

These are all I got in the Error Console when I put openDialog("about:crashes") in the Code bar and press Evaluate.

Comment 9

[timeless]

bp-e6c865b0-a6f2-46d0-ab44-8fd492090122
nsTextAddress::GetField
nsTextAddress::ProcessLine
nsTextAddress::ImportAddresses
ImportAddressImpl::ImportAddressBook
ImportAddressThread

Comment 10

[Joshua Cranmer [:jcranmer]]

Multiple stack traces -> this bug is confirmed.

Well, I think I found the problem:
http://mxr.mozilla.org/comm-central/source/mailnews/import/text/src/nsTextAddress.cpp?mark=287-310#287.

*moves to Import*

Comment 11

[David Ascher (:davida)]

joshua says he wants it.  thanks joshua!

Comment 12

[Joshua Cranmer [:jcranmer]]

I think I have something that fixes this, but I'm not able to write a testcase that cases a crash. Could you send me a crashing address book via private email?

Comment 13

[Joshua Cranmer [:jcranmer]]

Attachment: Preliminary version of the patch

I don't know if this still applies anymore, but this is the patch sitting in my mq directory that I think fixes this. I say think because I can't actually get it to fail without this patch, so I can't say if it fixes the problem.

Comment 14

[Mark Banner (:standard8)]

The Thunderbird drivers wish to release Thunderbird 3 as soon as possible. As a
result, we feel that this bug shouldn't stand in the way of all the other good
work getting into the hands of users sooner rather than later. Therefore we are
retargeting it for 3.1. See http://ccgi.standard8.plus.com/blog/archives/242
for more details. The 3.1 release is expected to be a quick release soon after
Thunderbird 3.

Comment 15

[Wayne Mery (:wsmwk)]

Joshua, if you need a testcase and Gabriel can't provide, there are a couple crash-stats reports with email addresses that I could ping to try to get something.

#32 crash and climbing (for 3.0.0 per 21 days)


from bp-c309f26f-afd2-41c1-bbf3-87ec92091216
0	thunderbird.exe	nsTextAddress::GetField	 mailnews/import/text/src/nsTextAddress.cpp:299
1	thunderbird.exe	nsTextAddress::ProcessLine	mailnews/import/text/src/nsTextAddress.cpp:471
2	thunderbird.exe	nsTextAddress::ImportAddresses	mailnews/import/text/src/nsTextAddress.cpp:123
3	thunderbird.exe	ImportAddressImpl::ImportAddressBook	mailnews/import/text/src/nsTextImport.cpp:516
4	thunderbird.exe	ImportAddressThread	mailnews/import/src/nsImportAddressBooks.cpp:1039

Comment 16

[Wayne Mery (:wsmwk)]

i've emailed the 3 crash reporters that gave addresses in last 7 months of crashes. (crash is now #29)

Comment 17

[Wayne Mery (:wsmwk)]

strike 1 - raul from bp-9f546a9d-b586-480c-a653-f8ff62091216 cannot reproduce

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Given what we know now, I don't think we'd hold 3.1 for this bug if it were the last one standing.  Wayne, if you disagree, feel free to renominate...

Comment 19

[Makoto Kato [:m_kato]]

http://mxr.mozilla.org/comm-central/source/mailnews/import/text/src/nsTextAddress.cpp?mark=300-303#300

281 bool nsTextAddress::GetField( const char *pLine, PRInt32 maxLen, PRInt32 index, nsCString& field, char delim)
282 {
 :
 :
300         if (*pChar == '"') {
301             len = -1;
302             do {


So len is reset even if pChar isn't reset.  so this will be possible overrun under this situation...  I cannot make sense why len sets -1.

Comment 20

[Makoto Kato [:m_kato]]

Attachment: Part 1. Thunderbird crash when importing address book that has quoted string

Comment 21

[Makoto Kato [:m_kato]]

Attachment: Part 2. fix leak on unit test

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 607870 [details] [diff] [review]
Part 1. Thunderbird crash when importing address book that has quoted string

Review of attachment 607870 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/import/test/unit/test_bug_474756.js
@@ +5,5 @@
> +function run_test()
> +{
> +  let abMgr = Cc["@mozilla.org/abmanager;1"].getService(Ci.nsIAbManager);
> +  let file = do_get_file("resources/bug_474756.txt");
> +  new AbImportHelper(file, "TAB", "bug_474756", "bug_474756").beginImport();

The 3rd and 4th arguments are not needed here if this test is only for the crash case.

Comment 23

[Makoto Kato [:m_kato]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> Comment on attachment 607870 [details] [diff] [review]
> Part 1. Thunderbird crash when importing address book that has quoted string
> 
> Review of attachment 607870 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/import/test/unit/test_bug_474756.js
> @@ +5,5 @@
> > +function run_test()
> > +{
> > +  let abMgr = Cc["@mozilla.org/abmanager;1"].getService(Ci.nsIAbManager);
> > +  let file = do_get_file("resources/bug_474756.txt");
> > +  new AbImportHelper(file, "TAB", "bug_474756", "bug_474756").beginImport();
> 
> The 3rd and 4th arguments are not needed here if this test is only for the
> crash case.

No.  checkResults will throw exception if nothing.

Comment 24

[David :Bienvenu]

Comment on attachment 607870 [details] [diff] [review]
Part 1. Thunderbird crash when importing address book that has quoted string

this new test doesn't fail for me, w/o the bug fix part of the patch applied. The intent was that it should, right?

Comment 25

[David :Bienvenu]

I should say, I tried this on windows, with both release and debug builds

Comment 26

[David :Bienvenu]

Comment on attachment 607870 [details] [diff] [review]
Part 1. Thunderbird crash when importing address book that has quoted string

r+ on the fix, but not on the test, which seems to be missing data...and doesn't fail w/o the patch.

Making reading off the end of memory is hard to make crash w/o using a memory mapped file or some such.

Comment 27

[Ludovic Hirlimann [:Usul]]

Joshua why is this still open ?

Comment 28

[Joshua Cranmer [:jcranmer]]

(In reply to Ludovic Hirlimann [:Usul] from comment #27)
> Joshua why is this still open ?

I don't know, but it looks like bienvenu had issues with Makoto's patch.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

(In reply to Ludovic Hirlimann [:Usul] from comment #27)
> Joshua why is this still open ?

Because the test in the patch is useless. We should land the patch without the test.

Comment 30

[David :Bienvenu]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)

> Because the test in the patch is useless. We should land the patch without
> the test.

yes, my issue was with the test, not the patch itself. Sorry if that wasn't clear.

Comment 31

[David :Bienvenu]

I'll try to come up with a cumulative patch, minus the unit test, and land it in the next day or two.

Comment 32

[Ludovic Hirlimann [:Usul]]

David ?

Comment 33

[David :Bienvenu]

fixed on trunk - I combined the leak fix and the crash fix w/o the test changes - http://hg.mozilla.org/comm-central/rev/5c2ee73ecb7b