1466343 - Subject of message displays incorrecly in thread pane if subject ends with RFC2047 token =?

Status: RESOLVED FIXED

(MailNews Core :: General, defect)

Description

[Bob Richardson]

Attachment: bug.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.181 Safari/537.36

Steps to reproduce:

I sent an email. You can see the subject. i'm using beta version 60.0b6 (32-bit)


Actual results:

The subject shown is a string of a characters (with o symbol).  The attached JPG shows the problem clearly. (email address covered in red for security).


Expected results:

The correct subject "Re: 1N-2C-2D-2S=?" can be seen  

Could this have been caused by ending the subject with a ? .

Comment 1

[Jorg K (CEST = GMT+2)]

Confirmed in TB 52 and TB 60 beta. TB 52 shows replacement characters, �, TB 60 shows those ångström characters.

=? is the opening token of an RFC2047 sequence. The strange thing is that the mis-decoding only happens in the thread pane.

Comment 2

[Jorg K (CEST = GMT+2)]

Another fact: It only happens in replies, when "Re: " is part of the message. Works fine with new messages and subject "1N-2C-2D-2S=?". So this is related to the special "Re: " processing.

Comment 3

[Jorg K (CEST = GMT+2)]

Looks like the cause is here
https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mailnews/base/util/nsMsgUtils.cpp#652
in NS_MsgStripRE(). The =? without closing ?= trips up that logic :-(

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1466343-NS_MsgStripRE.patch

Reviewer, the simple test case is this:
Create a new message, paste |Re: 1N-2C-2D-2S=?| into the subject. Save. Look at the draft in the draft folder.

For a laugh:
https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mailnews/base/util/nsMsgUtils.h#100

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1466343-NS_MsgStripRE.patch (v2)

OK, NS_MsgStripRE() was a total mess. It tried to extract the charset and then never used that value since the header was re-encoded in UTF-8 anyway.

I cleaned out most of the junk and switched to proper strings.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1466343-TestMsgStripRE.patch

I'm trying to get the tests to go, at least manually. They compile but don't link. Tom, do you know how to achieve this?

I must have built TestMailCookie.exe once upon a time, bug 1320861 comment #7, but I didn't take any notes :-(

Comment 7

[Jorg K (CEST = GMT+2)]

Philipp, can you tell me how to get the test program to link?

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8982848 [details] [diff] [review]
1466343-TestMsgStripRE.patch

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

::: mailnews/base/test/moz.build
@@ +19,5 @@
>  
>  LOCAL_INCLUDES += [
>      '/%s/netwerk/test' % CONFIG['mozreltopsrcdir'],
>      '/%s/xpcom/tests' % CONFIG['mozreltopsrcdir'],
>  ]

I tried adding:
USE_LIBS += [
    'mozglue',
    'xul',
]

OS_LIBS += CONFIG['MOZ_ZLIB_LIBS']
but still get heaps of unresolved external references when linking.

Comment 9

[Jorg K (CEST = GMT+2)]

Finally the thing is becoming a little clearer. TestMailCookie was ported to gtest in bug 1322072 which landed in Jan. 2017. I ran TestMailCookie before the port in Dec 2016 (bug 1320861 comment #7). Back then it wasn't a gtest.

So I think the way forward here is:
- Find out how to run TestMailCookie as a gtest.
- Port TestMsgStripRE.cpp to be a gtest as well.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1466343-TestMsgStripRE.patch (v2)

This resurrects TestMsgStripRE and converts it to a gtest.

It would be nice to work out how to run it. As mentioned in bug 1320861 comment #8, when running |mach gtest TestMsgStripRE*|, I got:
Error loading c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\bin\gtest/xul.dll: The specified module could not be found.

Copying a xul.dll to that directory then gives:
TEST-UNEXPECTED-FAIL | gtest | Not compiled with enable-tests
That's surprising, since I'm always building with |ac_add_options --enable-tests|.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 8982845 [details] [diff] [review]
1466343-NS_MsgStripRE.patch (v2)

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

LGTM, r=mkmelin

Comment 12

[Jorg K (CEST = GMT+2)]

OK, I'll land the fix and then we see how to resurrect the test. BTW, the test needs to be changed since after decoding we always re-encode in UTF-8, yet the test, which must pre-date JS Mime, expects the original charset.

Comment 13

[Jorg K (CEST = GMT+2)]

Actually, this is a blatant "read after free", so really a security issue.

Comment 14

[Jorg K (CEST = GMT+2)]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/629f8f4c79f9db0ec17e905478e25f5fe5b60208
fix NS_MsgStripRE(). r=mkmelin

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8982845 [details] [diff] [review]
1466343-NS_MsgStripRE.patch (v2)

[Triage Comment]

Comment 16

[Nicholas Nethercote [inactive]]

It's not clear to me what I'm being asked here. If it's about how to run a gtest, I would suggest this:

> mach gtest '*TestMsgStripRE*'

Comment 17

[Jorg K (CEST = GMT+2)]

TB 60 beta 7 (BETA_60_CONTINUATION branch):
https://hg.mozilla.org/releases/comm-beta/rev/d332994066acfec292b8ead77163c6ea10a315b1

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to Nicholas Nethercote [:njn] from comment #16)
> It's not clear to me what I'm being asked here. If it's about how to run a
> gtest, I would suggest this:
> > mach gtest '*TestMsgStripRE*'
Indeed, but that doesn't work, see comment #10. You could also take a look at attachment 8982906 [details] [diff] [review] which has f?njn.

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8982906 [details] [diff] [review]
1466343-TestMsgStripRE.patch (v2)

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

The code looks reasonable to me. I don't know about the problems with running it, alas.

::: mailnews/base/test/TestMsgStripRE.cpp
@@ +80,5 @@
>                           testInfoStructs[i].expectedOutput,
>                           testInfoStructs[i].expectedDidModify);
>      if (result)
>      {
> +      printf("Failed: %s, i=%d | result=%d\n", __FILE__, i, result);

You can do custom failure messages by using the '<<' operator with EXPECT_TRUE and other gtest macros/functions, e.g.:

> EXPECT_TRUE(result == 0) << "Got non-zero result: " << result;

@@ +88,4 @@
>    }
>  
>    if (allTestsPassed) {
> +    printf("all tests passed\n");

I don't think you need this.

Comment 20

[Jorg K (CEST = GMT+2)]

Thanks Nicholas. I'll take this to a new bug. The test wouldn't work anyway, see comment #12.

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 8982906 [details] [diff] [review]
1466343-TestMsgStripRE.patch (v2)

Moved to bug 1466748.

Comment 24

[Jorg K (CEST = GMT+2)]

TB 52.9:
https://hg.mozilla.org/releases/comm-esr52/rev/ff574714a516cb92ca6dc5fae061a905fc115a40

Comment 25

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/7318bc0281a5