344710 - Thunderbird GA-IE Crashes when entering IMAP password [@ nsCRT::strlen]

Status: RESOLVED FIXED

(Mozilla Localizations :: Other, defect)

Description

[Marcia Knous [:marcia]]

Seen using Tbird 1.5.0.5 (20060706). Both Juan and I see this on Intel Mac. On Windows I don't get the crash but it won't accept my valid password.

STR:

1. Download the GA-IE build from the candidates directory.
2. Click on your IMAP account to check your mail and enter your mail.
3. Immediate crash.

On Windows I don't get any errors in the JS console. Apple Crash report forthcoming.

Comment 1

[Marcia Knous [:marcia]]

Attachment: Apple crash report

Comment 2

[Marcia Knous [:marcia]]

adding axel to this bug and nominating.

Comment 3

[Scott MacGregor]

Related to Bug #334691 ? 

Comment 4

[Scott MacGregor]

I went through all of the strings that can be used by nsImapProtocol::ShowProgress (via nsImapProtocol::SetProgressString and the ga-IE translations for those strings all had the right number of %S / %lu's. 

For reference I looked at:

IMAP_FOLDER_RECEIVING_MESSAGE_OF
IMAP_RECEIVING_MESSAGE_HEADERS_OF
IMAP_RECEIVING_MESSAGE_FLAGS_OF

The strings are located:

http://lxr.mozilla.org/l10n-mozilla1.8.0/source/ga-IE/mail/chrome/messenger/imapMsgs.properties
http://lxr.mozilla.org/mozilla1.8.0/source/mail/locales/en-US/chrome/messenger/imapMsgs.properties

Comment 5

[Scott MacGregor]

The windows build crashes for me too and brings up talkback. Similar stack trace.

TB20962779

Comment 6

[Scott MacGregor]

This bug is a dupe of Bug #334691 which kept us from releasing ga-IE for 1.5.0.2. The stack traces are identical, and it looks like that bug never got fixed.

Comment 7

[Daniel Veditz [:dveditz]]

We're crashing in a home-grown strlen on a "while (*s++ != 0)" -- seems like the only way to crash there is to either be passed a bogus pointer in the first place, or a non-terminated one that increments until we trigger an access violation.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/ds/nsCRT.cpp&mark=180&rev=MOZILLA_1_8_0_BRANCH#180

The line calling that is the cvt_S() dealing with the %S replacement -- apparently that is not a string.

Now one interesting thing is that the order of replacements here is different, "%lu %lu %S" instead of "%S %lu %lu" in the other languages I looked at. printf() certainly doesn't let you do that, and would generally crash -- ah ha!

The replacement variables need to use the "re-ordering" syntax: "%2$lu %3$lu %1$S" (or %3$lu %2$lu if they reorder the meaning of those as well).

The number in 'n$' is the index into the argument array, which is compiled into the source and will have the same ordering in all languages. When a translation reorders things they *MUST* specify which argument they intend in the original order.

Not sure what happens if some placeholders have an index and some don't, I don't recommend trying to mix the styles.

Comment 8

[Scott MacGregor]

I've been able to reproduce this in the debugger with a ga-IE build. The string in question is:

http://lxr.mozilla.org/l10n-mozilla1.8.0/source/ga-IE/mail/chrome/messenger/imapMsgs.properties#192

This string has 3 format variables: a %S followed by a %lu and another %lu.

ga-IE looks like:

Ceannt\u00E1sc %lu as %lu \u00E1 \u00EDoslucht\u00FA \u00F3 %S

en-us looks like:
%S Downloading message header %lu of %lu

It looks like we are crashing while trying to format a number as a string because the variable order has changed in the localization.

I see a couple locales that re-arrange the order of these variables. But those locales end up numbering the variables. i.e. mk which does:
%2$lu %3$lu %1$lu

I think if you change the order of the parameters you need to number them like this example.

I've noticed there are other strings in this file where the %S was moved after the %lu as well. So we'd need to fix more than just this one string in the localization. 

Comment 9

[Scott MacGregor]

err this is pretty much the same conclusion Dan came up with earlier. :)

Comment 10

[Daniel Veditz [:dveditz]]

> Not sure what happens if some placeholders have an index and some don't, I
> don't recommend trying to mix the styles.

In the same string, I meant.

Comment 11

[Kevin Scannell]

Attachment: reordering variables in imapMsgs.properties

Thanks for the debugging help everyone.  I'm attaching a patch that adds the
reordering to the two offending strings in imapMsgs.properties.
I grepped for other possible problems and didn't find anything.

I'm not sure which approval flag I need to set for the 1.8.0 branch.

In any case, I assume I can apply the same patch to 1.8 and trunk
(when the trees reopen) without additional approval?

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 229365 [details] [diff] [review]
reordering variables in imapMsgs.properties

r=me for the fix itself, requesting 1.8.0.5 approval, as that is the release this bug is blocking.

Please land this on trunk and 1.8 branch as soon as the trees open.

Comment 13

[Jay Patel [:jay]]

Comment on attachment 229365 [details] [diff] [review]
reordering variables in imapMsgs.properties

approved for 1.8.0 branch, a=jay for drivers.  

Please check in asap.

Comment 14

[Kevin Scannell]

(In reply to comment #13)
> 
> Please check in asap. 
> 

Checked into 1.8.0 branch.

Comment 15

[Daniel Veditz [:dveditz]]

This was checked into the 1.8.0 branch (dunno about other places so I'll leave it open in general).

Comment 16

[Kevin Scannell]

(In reply to comment #15)
> This was checked into the 1.8.0 branch (dunno about other places so I'll leave
> it open in general).

Also checked in to 1.8 branch and the trunk earlier today. 

Comment 17

[Jay Patel [:jay]]

I tried verifying this fix, but I am not able to reproduce the crash on Mac Intel with either rc3 or rc4 (20060719 build).  Checking IMAP mail prompts me for my password, I enter it (tried both checking the "remember" checkbox and leaving it unchecked), and my mail account works as expected.

Juan or Marcia:  Can you try to reproduce this again and see if the crash has gone  away with the latest rc4 builds?

Comment 18

[Scott MacGregor]

Jay, your imap account needs to have new mail in it, and then you'll be able to reproduce the crash. cheers.

Comment 19

[Jay Patel [:jay]]

Ahh... thanks Scott!  I sent myself new mail and after starting 1.5.0.5 rc3 I got a crashed waiting for my IMAP mail to download.  It didn't have anything to do with entering my pw. ;-)
"
No crash with 1.5.0.5 rc4.  v.fixed with "leagan 1.5.0.5 (20060719)"

Comment 20

[Marcia Knous [:marcia]]

I tested this as well, I don't see any crash on the Intel Mac in the QA lab. Double verification can't hurt :). Resolving this as fixed to get it off the radar.