1412213 - Message body is not displayed if inline attachment header contains two semicolons

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[jaim3z]

Attachment: bug.eml

User Agent: Mozilla/5.0 (Windows NT 10.0; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170926190823

Steps to reproduce:

Received email message from certain senders with inline message attachment with small mistake in source code at line
Content-Type: multipart/related;type="multipart/alternative";;


Actual results:

Message body is not displayed 


Expected results:

It should throw some error message or view message body text. Other email clients like Outlook or Windows mail renders whole message and attachment correctly

Comment 1

[jaim3z]

Also, when I edit that EML file and delete one of the semicolons at that line message and attachment is displayed correctly. I manualy edited posted EML file and replaced original image. So, for some reason, when you try to correct that line too, image is displayed as normal attachment and not as inline.

Comment 2

[Wayne Mery (:wsmwk)]

searched for exact duplicate but found none

Comment 3

[Alfred Peters [:infofrommozilla]]

Attachment: Bug1412213_v1.patch Content-Type MIME-header - Fix of skipping an empty parameter

There is a glitch in nsMIMEHeaderParamImpl::DoParameterInternal() parsing an empty Content-Type header parameter.

<https://dxr.mozilla.org/comm-central/rev/32b850fa28ae1c29039cb7ddcdfd71b324762c05/mozilla/netwerk/mime/nsMIMEHeaderParamImpl.cpp#473>
| if (*str++ != '=') {
|   // don't accept parameters without "="
|   goto increment_str;

If the parameter is empty (';;') there is no '='.
So the jump to "increment_str" should skip this parameter and continue with the next one.
The "str++" skips over the second ';'. But this character is needed by the "increment_str" code to proceed to the next parameter. Since it is missing, the function is aborted and the remaining parameters are ignored.

The fix executes the "str++" only without the jump.
The patch displays the example correctly.

This is mozilla-central code. Who would be possible as reviewer?

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8946052 [details] [diff] [review]
Bug1412213_v1.patch Content-Type MIME-header - Fix of skipping an empty parameter

(In reply to Alfred Peters from comment #3)
> This is mozilla-central code. Who would be possible as reviewer?
Usually you refer to https://wiki.mozilla.org/Modules/Core and pick one of the peers as reviewer. I know Honza and Valentin from other bugs, so I'll add them. One review will do! ;-)

As a matter of interest, how do we get into this code? I thought, header parsing is done in TB code in JS Mime. Do you have a call stack? Or do we string together a big HTML document and pass that to M-C for display?

Comment 5

[Alfred Peters [:infofrommozilla]]

Attachment: Bug1412213_call_stack.txt

(In reply to Jorg K (GMT+1) from comment #4)
> As a matter of interest, how do we get into this code? I thought, header
> parsing is done in TB code in JS Mime. Do you have a call stack? Or do we

<https://dxr.mozilla.org/comm-central/rev/2b56d89a8792fc1af6ae66be5f38cd9d26ecb7e1/mailnews/mime/src/mimehdrs.cpp#500>

...this (and others) calls GetParameterInternal() from 'nsMIMEHeaderParamImpl.cpp'.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8946052 [details] [diff] [review]
Bug1412213_v1.patch Content-Type MIME-header - Fix of skipping an empty parameter

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

I don't feel comfortable to review any strstr/*str++ kind of code changes these days...  I would suggest to rewrite this using mozilla::Tokenizer from scratch, but might be an overkill (the parser seems complicated, but maybe it's just because it's a *str++ kind of parsing code :))).

Anyway, regardless of which way is chosen here, I won't accept such a patch w/o a test for the particular case this is being fixed for.

r- for lack of a test.

Also, please read the https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch docs before you submit next time.

Thanks!

::: netwerk/mime/nsMIMEHeaderParamImpl.cpp
@@ +474,4 @@
>        // don't accept parameters without "="
>        goto increment_str;
>      }
> +    str++;

if at least.. this really needs a very good comment why it can/has to be here

Comment 7

[Jorg K (CEST = GMT+2)]

I think this is a bit harsh, Alfred made a good effort to track down the problem. Yes, a big comment would be good.

As for the test, maybe test_MIME_params.js could be extended or taken as a template for a new test.

Comment 8

[Alfred Peters [:infofrommozilla]]

Attachment: Fix of skipping a parameter without a =

(In reply to Honza Bambas (:mayhemer) from comment #6)

> Anyway, regardless of which way is chosen here, I won't accept such a patch
> w/o a test for the particular case this is being fixed for.

I added a test. And a second, in the case of a parameter without =.
Both tests pass with the patch and fail without it.

> > +    str++;
> 
> if at least.. this really needs a very good comment why it can/has to be here

I did that too.

Comment 9

[Alfred Peters [:infofrommozilla]]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> I don't feel comfortable to review any strstr/*str++ kind of code changes
> these days...  I would suggest to rewrite this using mozilla::Tokenizer from
> scratch, but might be an overkill (the parser seems complicated, but maybe

Agreed, but we should do that in a new bug...

Comment 10

[Alfred Peters [:infofrommozilla]]

Attachment: Fix of skipping a parameter without a =

Only a few space corrections.

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8946444 [details] [diff] [review]
Fix of skipping a parameter without a =

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

Thanks!

I've pushed to try as I tried the patch locally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=20823b0cbdc8b293ed26dbc5f25b552a8c0f81f0

Comment 12

[Ryan VanderMeulen [:RyanVM]]

For future reference, please try to use commit messages in your patches that summarize what the patch is actually doing, not just restating the problem being fixed. Thanks!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc8174e0c380
Content-Type MIME-header - Skipping a parameter without an '='. r=mayhemer

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/bc8174e0c380