Closed Bug 1466343 Opened 3 years ago Closed 3 years ago

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

Categories

(MailNews Core :: General, defect)

defect
Not set
normal

Tracking

(thunderbird_esr52 fixed, thunderbird_esr60 fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 fixed)

RESOLVED FIXED
Thunderbird 62.0
Tracking Status
thunderbird_esr52 --- fixed
thunderbird_esr60 --- fixed
thunderbird60 --- fixed
thunderbird61 --- wontfix
thunderbird62 --- fixed

People

(Reporter: bob, Assigned: jorgk-bmo)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached image 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 ? .
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Subject of sent emails messed up → Subject of message displays incorrecly in thread pane of subject ends with RFC2047 token =?
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.
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 :-(
Attached patch 1466343-NS_MsgStripRE.patch (obsolete) — Splinter Review
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
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8982841 - Flags: review?(mkmelin+mozilla)
Attachment #8982841 - Flags: review?(acelists)
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.
Attachment #8982841 - Attachment is obsolete: true
Attachment #8982841 - Flags: review?(mkmelin+mozilla)
Attachment #8982841 - Flags: review?(acelists)
Attachment #8982845 - Flags: review?(mkmelin+mozilla)
Attachment #8982845 - Flags: review?(acelists)
Attached patch 1466343-TestMsgStripRE.patch (obsolete) — Splinter Review
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 :-(
Attachment #8982848 - Flags: feedback?(mozilla)
Philipp, can you tell me how to get the test program to link?
Flags: needinfo?(philipp)
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.
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.
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|.
Attachment #8982848 - Attachment is obsolete: true
Attachment #8982848 - Flags: feedback?(mozilla)
Flags: needinfo?(philipp)
Attachment #8982906 - Flags: feedback?(n.nethercote)
Comment on attachment 8982845 [details] [diff] [review]
1466343-NS_MsgStripRE.patch (v2)

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

LGTM, r=mkmelin
Attachment #8982845 - Flags: review?(mkmelin+mozilla)
Attachment #8982845 - Flags: review?(acelists)
Attachment #8982845 - Flags: review+
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.
Keywords: leave-open
Actually, this is a blatant "read after free", so really a security issue.
Group: mail-core-security
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/629f8f4c79f9db0ec17e905478e25f5fe5b60208
fix NS_MsgStripRE(). r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 62.0
Comment on attachment 8982845 [details] [diff] [review]
1466343-NS_MsgStripRE.patch (v2)

[Triage Comment]
Attachment #8982845 - Flags: approval-comm-esr60+
Attachment #8982845 - Flags: approval-comm-beta+
Summary: Subject of message displays incorrecly in thread pane of subject ends with RFC2047 token =? → Subject of message displays incorrecly in thread pane if subject ends with RFC2047 token =?
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*'
(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 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.
Attachment #8982906 - Flags: feedback?(n.nethercote) → feedback+
Thanks Nicholas. I'll take this to a new bug. The test wouldn't work anyway, see comment #12.
Component: Untriaged → General
Product: Thunderbird → MailNews Core
Blocks: 1466748
Comment on attachment 8982906 [details] [diff] [review]
1466343-TestMsgStripRE.patch (v2)

Moved to bug 1466748.
Attachment #8982906 - Attachment is obsolete: true
Group: mail-core-security → core-security-release
Duplicate of this bug: 1467990
Duplicate of this bug: 1470013
Depends on: 1476788
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.