243854 - MIME hang while sanitizing a message

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Ere Maijala (slow)]

I received a message that caused Mozilla to hang completely. Test mbox and patch
coming up.

Comment 1

[Ere Maijala (slow)]

Attachment: mailbox file of the offending message

This is a mailbox file containing the offending message. Hangs at least if
viewed in Simple HTML.

Comment 2

[Ere Maijala (slow)]

Attachment: Patch v1

This patch makes sure the string given to MimeInlineTextPlainFlowed_parse_line
is null-terminated. It also prevents an assertion when ScanTXT was called with
an empty line.

Comment 3

[Ere Maijala (slow)]

This should be fixed in the 1.7 branch too. 

Comment 4

[David :Bienvenu]

thx for looking at this?

are you sure this won't write past the end of our memory block?

+        (*bufferP)[*buffer_fpP] = '\0';

re the second part, I think it would be better to wrap the whole

if (length - (linep - line)) check were moved up to surround that whole block of
code, like this:
  if (length - (linep - line) > 0)
  {
    nsDependentCString inputStr(linep, length - (linep - line));

    // For 'SaveAs', |line| is in |mailCharset|.
    // convert |line| to UTF-16 before 'html'izing (calling ScanTXT())

    ....
    rv = conv->ScanTXT(lineSource.get(), whattodo, getter_Copies(lineResult));
    NS_ENSURE_SUCCESS(rv, -1);
   }

Does that work?

Comment 5

[David :Bienvenu]

it looks like we've allocated an extra byte, so the null assignment shouldn't
crunch anything...

Comment 6

[Ere Maijala (slow)]

Attachment: Patch v2 (without whitespace changes)

Yes, it should be safe to write the null there. 

The length check could be moved even higher, to the beginning of that block.

Comment 7

[Ere Maijala (slow)]

Attachment: Patch v2

Comment 8

[Ere Maijala (slow)]

Comment on attachment 148696 [details] [diff] [review]
Patch v2 (without whitespace changes)

Sorry, can I get an r= too, please? 

Requesting also approval for this small but significant fix.

Comment 9

[David :Bienvenu]

Comment on attachment 148696 [details] [diff] [review]
Patch v2 (without whitespace changes)

r=bienvenu - I've cc'd ducarroz, in case he has some thoughts on this.

Comment 10

[Ere Maijala (slow)]

Thanks. For the record: the terminating null is needed because
MimeInlineTextPlainFlowed_parse_line uses nsDependentCString. Otherwise length
checks in two other places would have been enough.

Comment 11

[Ere Maijala (slow)]

Trunk fixed.

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 148696 [details] [diff] [review]
Patch v2 (without whitespace changes)

a=asa (on behalf of drivers) for checkin to 1.7

Comment 13

[Ere Maijala (slow)]

Fixed in branch too. Thanks to Neil for the checkin.