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

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jaim3z, Assigned: infofrommozilla)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Posted file 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
(Reporter)

Comment 1

2 years ago
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.
searched for exact duplicate but found none
(Assignee)

Comment 3

a year ago
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

a year ago
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?
Attachment #8946052 - Flags: review?(valentin.gosu)
Attachment #8946052 - Flags: review?(honzab.moz)
(Assignee)

Comment 5

a year ago
(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 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
Attachment #8946052 - Flags: review?(honzab.moz) → review-

Comment 7

a year ago
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.
(Assignee)

Updated

a year ago
Attachment #8946052 - Flags: review?(valentin.gosu)
(Assignee)

Comment 8

a year ago
(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.
Attachment #8946052 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8946357 - Flags: review?(honzab.moz)
(Assignee)

Comment 9

a year ago
(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...
(Assignee)

Updated

a year ago
Attachment #8946357 - Attachment is obsolete: true
Attachment #8946357 - Flags: review?(honzab.moz)
(Assignee)

Comment 10

a year ago
Only a few space corrections.
Attachment #8946444 - Flags: review?(honzab.moz)
Assignee: nobody → infofrommozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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
Attachment #8946444 - Flags: review?(honzab.moz) → review+
(Assignee)

Updated

a year ago
Component: Message Reader UI → Networking
Keywords: checkin-needed
OS: Unspecified → All
Product: Thunderbird → Core
Hardware: Unspecified → All
Version: 52 Branch → Trunk
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

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc8174e0c380
Content-Type MIME-header - Skipping a parameter without an '='. r=mayhemer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bc8174e0c380
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.