134277 - quoted-pair (e.g. '\"') in display-name doesn't get handled correctly

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Takayuki Tei]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; ja-JP; rv:0.9.9+)
Gecko/20020329
BuildID:    20020327

A couple of different issues:

1) Display issue.  I see raw '\' in thread pane and message pane which sould be
invisible.
2) If you reply to the message from sender with quoted-pair in display-name, or
one of recipient has the quoted-pair in display-name, some recipient may not
receive message.  A problem might occur at envelop level as well, but not sure.
3) Other MUA cannot deal with open quoted-string as a result of this.

Display issue is not that big deal.  but 2) and 3) are very nasty. The worst
thing is, user don't see any error message or such.  A message just disappears
that user believes it's sent correctly.

Reproducible: Always
Steps to Reproduce:
1. In mail&news pref, change your name to something like 'John "Fitzgerald"
Kennedy'.
2. Send a message to yourself.
3. Reply to it with more recipients.

Actual Results:  Newly added recipients won't receive message.

Expected Results:  All recipients should receive message.

Comment 1

[Takayuki Tei]

Attachment: fix for display issue *only*

Get rid of '\' in thread pane.

Comment 2

[Takayuki Tei]

fixed summary.

Comment 3

[scottputterman]

reassigning to ducarroz who is the MIME owner.

Comment 4

[nhottanscp]

Attachment: test case

Comment 5

[Michael Buckland]

Discussed in Mail News bug mtg with Engineering QA and PjM.  Decided to ADT2 and
plus this bug.

Comment 6

[Jean-Francois Ducarroz]

I have a fix for the send part (encoding), let me check the patch for the
display part (decoding) Taka made. Then I'll post the full patch...

Comment 7

[Takayuki Tei]

My fix is only applicable to thread pane.  Message pane is rendered by other
pieces of code and you have to decode quoted-pair appearing in structured
headers only in there. Do not touch if it's unstructured header (e.g. Subject:)
otherwise you change the meaning in different way.

As far as I can tell, encoding seems to be okay.

Comment 8

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v2

This patch include previous patch (with a little modification) and fix various
other display problem as well encoding problem during the send/save.

Comment 9

[Takayuki Tei]

Attachment: screen shot

Looks good in thread pane.
From line in message pane should not show '\'.
To line in message pane should not get rid of '"'.

Comment 10

[Jean-Francois Ducarroz]

did the screen shot was made using the patch I posted?

Comment 11

[Takayuki Tei]

yes.

Comment 12

[John G. Myers]

Could someone clarify where this quoted-pair is introduced?  Is this an RFC 2822
quoted-pair or is this an IMAP quoted-pair?

Comment 13

[Takayuki Tei]

RFC 2822.

Comment 14

[Takayuki Tei]

Another side effect by the patch was found.
Tried to reply to following sender:

  From: "ABC/SJC - Anderson, Someone" <[e-mail]>

This ended up as follows:

  To:  ABC/SJC,  -,  Anderson, Someone <[e-mail]>

Comment 15

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v3

I fix more problems:
1) we were losing quote during send in some case
2) quote wasn't always escaped in message display
3) quote wasn't escaped when doing a forward inline

Comment 16

[Thierry Carrez]

Related bug on encoding display names :

Please have a look at bug 137742 which shows that encoding of multiple display
names is incorrect. Example :

To: John Doe <[e-mail]>
To: Jane Doe <[e-mail]>

will get encoded as :

To: John Doe <[e-mail]>, Jane Doe[0D][0A] <[e-mail]>[0D][0A]

instead of :

To: John Doe <[e-mail]>, Jane Doe <[e-mail]>[0D][0A]

with Mozilla 1.0RC1 (and probably since 0.9.9)
This does confuse some MDA, including Netscape Messaging Server 3.X which will
drop addresses, resulting in data loss.

Comment 17

[Thierry Carrez]

Just a comment on my previous note :

In fact bug 137742 is about recipient header folding on multiple lines. The new
way it's done, from Mozilla 0.9.9 on, confuses some mail servers, although it's
a standard-compliant way. RFC 822 has a recommended way for headers folding
which is more widely accepted by MTAs : this was the way implemented in Mozilla
0.9.8 and before...

Comment 18

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v4

I started from scratch this time and this patch look much much better...

Comment 19

[Takayuki Tei]

Attachment: address test shell script

The message sent by this script should show up in MUA like below:

  From: ABC/SJC - "Anderson", Someone <[e-mail]>
  To: ABC/SJC - "Anderson", Someone <[e-mail]>
  To: Nippon Taro / [NIPPON TARO in Kanji] <[e-mail]>
  To: ABC/SJC - "Anderson", Someone <[e-mail]> (ABC/SJC - (Anderson)
Someone)
  To: [e-mail] (ABC/SJC - "Anderson", Someone)
  Subject: Addressing test

Comment 20

[Jean-Francois Ducarroz]

I tested the last patch using the provided script (had to enter manually all the
command as the script was failing to execute) and sofar I haven't see any problem.

Comment 21

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v5

I have address Taka comment made by email.

Comment 22

[Takayuki Tei]

Comment on attachment 81790 [details] [diff] [review]
Proposed fix, v5

r=taka

Comment 23

[David :Bienvenu]

I think you should add a comment as to the parameters here - address is both an
in and an out parameter, right?

+static void UnquoteMimeAddress(nsIMsgHeaderParser* parser, char** address)
+{
+  if (parser && address && *address && **address)
+  {
+    char *result;

there's lots of new commented out code /* */ and #if 0 code in this patch - it
would be easier to review, and cleaner, if that code weren't there. Is there a
reason for it?

here, unquotedName can be an nsXPIDLCString, right?

+    char * unquotedName = nsnull;
     while (index < numAddresses)
     {
+      if (NS_SUCCEEDED(UnquotePhraseOrAddr(currentName, PR_TRUE, &unquotedName)))
+        rv = FillResultsArray(unquotedName, currentAddress,
&outgoingEmailAddresses[index], &outgoingNames[index],
&outgoingFullNames[index], this);
+      else
       rv = FillResultsArray(currentName, currentAddress,
&outgoingEmailAddresses[index], &outgoingNames[index],
&outgoingFullNames[index], this);
+      
+      PR_FREEIF(unquotedName);



these strcpy's are a little scary because of buffer overrun attacks. I know
you've allocated space enough for every character to be escaped but are you
absolutely sure this is safe? Using strncpy would be safer.

+        reformated = msg_reformat_Header_addresses(startRecipient);
+        if (reformated)
+        {
+          strcpy(writePtr, reformated);
+          writePtr += strlen(reformated);
+          PR_Free(reformated);
+        }
+        else
+        {
+          strcpy(writePtr, startRecipient);
+          writePtr += strlen(startRecipient);
+        }


looks like tabs here:
+    {
+
	  *lineout = nsCRT::strdup(line);
+
	  if (!*lineout)
+
		  return NS_ERROR_OUT_OF_MEMORY;
 	else
-
	*lineout = NULL;
+
		  return NS_OK;
 I'm going to commit these comments, and look some more, and see if you want to
attach a patch w/o all the #if 0 stuff and commented out code.

Comment 24

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v6

I have addressed Bienvenu comments:

>I think you should add a comment as to the parameters here - address is both
an
>in and an out parameter, right?
done

>there's lots of new commented out code /* */ and #if 0 code in this patch - it

>would be easier to review, and cleaner, if that code weren't there. Is there a

>reason for it?
a Lots is not the appropiate word but anyway, that was left over I forget to
cleanup. Done

>here, unquotedName can be an nsXPIDLCString, right?
Yes it could but that won't really make the code cleaner or help to prevent
leak in that case. However, the overhead of using a nsXPIDLCString will have a
performance impact. Therefore it doesn't worst to use it.

>these strcpy's are a little scary because of buffer overrun attacks. I know
>you've allocated space enough for every character to be escaped but are you
>absolutely sure this is safe? Using strncpy would be safer.
Done

>looks like tabs here:
I have untabified the whole file but as the diff doesn't show the white space
changes, it looks messy.

Comment 25

[David :Bienvenu]

+
/* This function removes the quoting if you want to show the
+
   names to users. e.g. summary file, address book. If preserveIntegrity is set
to true,
+     quote will not be removed in case the name part of the email contains a comma.
 	 */
this looks like a real tab, not CVS messing up - these are lines you added, right?

I still see some strcpy's here:
+  if (reformated)
+  {
+    strcpy(writePtr, reformated);
+    PR_Free(reformated);
+  }
+  else
+    strcpy(writePtr, startRecipient);

Again, I think there must be tabs here (or the indentation is messed up):
+
				if (s < mailbox_start && (*s == '\\' || *s == '\"'))
+
			    *name_out++ = *s++;
+          continue;

and here:
+
				if (s < mailbox_end && (*s == '\\' || *s == '\"'))
+
				 	*addr_out++ = *s++;
+          continue;

Comment 26

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v7

I have convert the 2 remaining strcpy tp strncpy and converted old code that
was still using tabs to space. All the new lines was correctly useing space.
The diff will show old code with tab as the diff is done using the -w option.

Comment 27

[David :Bienvenu]

Comment on attachment 83598 [details] [diff] [review]
Proposed fix, v7

sr=bienvenu

Comment 28

[Jean-Francois Ducarroz]

Fix checked in the trunk.

I would not recommend to check this fix in the branch without having at least 2
weeks of live testing

Comment 29

[scottputterman]

marking nsbeta1- for rtm.

Comment 30

[John Levon]

er, is this bug 39955 ?

Comment 31

[Takayuki Tei]

Does this fix make it to Netscape's RTM?

Please note that some recipients may not receive message
if there is a quoted-pair in recipient list.

This is not merely cosmetic problem.

Comment 32

[Jean-Francois Ducarroz]

Not in the branch yet, this is kind of too risky at this point...

Comment 33

[Isaac Rabinovitch]

*** Bug 117975 has been marked as a duplicate of this bug. ***

Comment 34

[Boris 'pi' Piwinger]

Did this fix here cause bug 189928?

pi

Comment 35

[Mike Cowperthwaite]

*** Bug 39955 has been marked as a duplicate of this bug. ***