435587 - Crash [@ nsMsgCompose::ProcessSignature] when htmlSigText is set but no signature file is given

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[rsx11m]

While testing for bug 428040, this scenario would crash on current trunk: Set the htmlSigText preference for an identity with a valid signature string, but leave "Attach this signature:" unchecked, then compose a new message.

The apparent reason is that the handling of the recently introduced preference "mail.identity.id*.htmlSigText" allows code accessing the signature file to be executed where it would not before. That code relies on the presence of the file, thus additional conditions have to be introduced to cover those cases.

Comment 1

[rsx11m]

Attachment: Quick fix

This patch initializes PRBool exists correctly and adds further checks on "exists" before executing code processing the signature file.

Comment 2

[[e-mail]]

Comment on attachment 322383 [details] [diff] [review]
Quick fix

I can't seem to find the bug where this code was added :-( I wonder whether the exists check should occur before checking the mime type.

Comment 3

[rsx11m]

Neil: According to bonsai, the htmlSigText pref was introduced with revisions 1.504-1.507 by bug 33451, attachment 260305 [details] [diff] [review]. This bug has a rather large
collection of patches, but I couldn't find any comment on this specific pref.
The checking of the MIME type before the existence check was introduced by
bug 225570, revision 1.412-1.413 of nsMsgCompose.cpp.

Anyway, even if GetTypeFromFile() catches the non-existing filename case, it's certainly cleaner to test this upfront. Also, the "exists" variable appears redundant as a global variable and can be combined with "useSigFile".

Comment 4

[rsx11m]

Attachment: Better fix (v2, before white-space adjustment)

Informational. The "exists" variable is now a local variable and "useSigFile" will not be set PR_TRUE if the file doesn't exists. Also, the MIME-type check won't be executed either in this case. All following checks are consolidated, using "useSigFile" only, to simplify the code.

Comment 5

[rsx11m]

Attachment: Better fix (v2)

Patch from attachment 322533 [details] [diff] [review] after adjustment of white spaces.
Carrying forward sr+ from attachment 322383 [details] [diff] [review].

Comment 6

[[e-mail]]

Comment on attachment 322533 [details] [diff] [review]
Better fix (v2, before white-space adjustment)

>diff -u -8 -p -r1.561 nsMsgCompose.cpp
Normally when creating a patch that involves significant whitespace changes (as here) it's best to write the final patch but create a second diff including -w.

>-  if (imageSig)
>+  if (useSigFile && imageSig)
I think your new code ensures that imageSig is only set after useSigFile is.

Comment 7

[rsx11m]

Attachment: Better fix (v3)

(In reply to comment #6)
> I think your new code ensures that imageSig is only set after useSigFile is.

Oops, I've missed that. Since imageSig is initialized PR_FALSE, it can't be true unless useSigFile applies and we checked the MIME type. Thanks.

Comment 8

[Phil Ringnalda (:philor)]

Comment on attachment 322569 [details] [diff] [review]
Better fix (v3)

Sorry, I can only review in mail/, for mailnews/ you want someone from http://www.mozilla.org/owners.html#mail-and-news-backend

Comment 9

[rsx11m]

Thanks Mark, checkin for trunk please.

Comment 10

[Mark Banner (:standard8)]

Checking in mailnews/compose/src/nsMsgCompose.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v  <--  nsMsgCompose.cpp
new revision: 1.562; previous revision: 1.561
done

Comment 11

[rsx11m]

-> FIXED