Closed
Bug 134277
Opened 23 years ago
Closed 22 years ago
quoted-pair (e.g. '\"') in display-name doesn't get handled correctly
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.0
People
(Reporter: mozilla, Assigned: bugzilla)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [ADT2] have fix,)
Attachments
(4 files, 6 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•23 years ago
|
||
Get rid of '\' in thread pane.
Reporter | ||
Comment 2•23 years ago
|
||
fixed summary.
Summary: quoted-pair (e.g. '\"' in display-name doesn't get handled correctly → quoted-pair (e.g. '\"') in display-name doesn't get handled correctly
Updated•23 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•23 years ago
|
QA Contact: gayatri → olgam
Comment 4•23 years ago
|
||
Comment 5•23 years ago
|
||
Discussed in Mail News bug mtg with Engineering QA and PjM. Decided to ADT2 and
plus this bug.
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•23 years ago
|
||
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...
Reporter | ||
Comment 7•23 years ago
|
||
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.
Assignee | ||
Comment 8•23 years ago
|
||
This patch include previous patch (with a little modification) and fix various
other display problem as well encoding problem during the send/save.
Assignee | ||
Updated•23 years ago
|
Whiteboard: [ADT2] → [ADT2] have fix
Reporter | ||
Comment 9•23 years ago
|
||
Looks good in thread pane.
From line in message pane should not show '\'.
To line in message pane should not get rid of '"'.
Assignee | ||
Comment 10•23 years ago
|
||
did the screen shot was made using the patch I posted?
Reporter | ||
Comment 11•23 years ago
|
||
yes.
Comment 12•23 years ago
|
||
Could someone clarify where this quoted-pair is introduced? Is this an RFC 2822
quoted-pair or is this an IMAP quoted-pair?
Reporter | ||
Comment 13•23 years ago
|
||
RFC 2822.
Reporter | ||
Comment 14•23 years ago
|
||
Another side effect by the patch was found.
Tried to reply to following sender:
From: "ABC/SJC - Anderson, Someone" <Someonei.Anderson@sjc.abccompany.com>
This ended up as follows:
To: ABC/SJC, -, Anderson, Someone <Someone.Anderson@sjc.abccompany.com>
Assignee | ||
Comment 15•23 years ago
|
||
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
Attachment #76781 -
Attachment is obsolete: true
Attachment #78663 -
Attachment is obsolete: true
Comment 16•23 years ago
|
||
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 <jdoe@tc.com>
To: Jane Doe <jane@dot.com>
will get encoded as :
To: John Doe <jdoe@tc.com>, Jane Doe[0D][0A] <jane@dot.com>[0D][0A]
instead of :
To: John Doe <jdoe@tc.com>, Jane Doe <jane@dot.com>[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•22 years ago
|
||
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...
Assignee | ||
Comment 18•22 years ago
|
||
I started from scratch this time and this patch look much much better...
Attachment #79746 -
Attachment is obsolete: true
Reporter | ||
Comment 19•22 years ago
|
||
The message sent by this script should show up in MUA like below:
From: ABC/SJC - "Anderson", Someone <foo@bar.com>
To: ABC/SJC - "Anderson", Someone <foo@bar.com>
To: Nippon Taro / [NIPPON TARO in Kanji] <foo@bar.com>
To: ABC/SJC - "Anderson", Someone <foo@bar.com> (ABC/SJC - (Anderson)
Someone)
To: foo@bar.com (ABC/SJC - "Anderson", Someone)
Subject: Addressing test
Assignee | ||
Comment 20•22 years ago
|
||
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.
Assignee | ||
Comment 21•22 years ago
|
||
I have address Taka comment made by email.
Attachment #80995 -
Attachment is obsolete: true
Reporter | ||
Comment 22•22 years ago
|
||
Comment on attachment 81790 [details] [diff] [review]
Proposed fix, v5
r=taka
Attachment #81790 -
Flags: review+
Assignee | ||
Updated•22 years ago
|
Whiteboard: [ADT2] have fix → [ADT2] have fix, need SR
Comment 23•22 years ago
|
||
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.
Assignee | ||
Comment 24•22 years ago
|
||
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.
Attachment #81790 -
Attachment is obsolete: true
Comment 25•22 years ago
|
||
+
/* 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;
Assignee | ||
Comment 26•22 years ago
|
||
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.
Attachment #83105 -
Attachment is obsolete: true
Comment 27•22 years ago
|
||
Comment on attachment 83598 [details] [diff] [review]
Proposed fix, v7
sr=bienvenu
Attachment #83598 -
Flags: superreview+
Assignee | ||
Comment 28•22 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Whiteboard: [ADT2] have fix, need SR → [ADT2] have fix,
Comment 30•22 years ago
|
||
er, is this bug 39955 ?
Reporter | ||
Comment 31•22 years ago
|
||
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.
Assignee | ||
Comment 32•22 years ago
|
||
Not in the branch yet, this is kind of too risky at this point...
Comment 33•22 years ago
|
||
*** Bug 117975 has been marked as a duplicate of this bug. ***
Comment 34•22 years ago
|
||
Did this fix here cause bug 189928?
pi
Comment 35•21 years ago
|
||
*** Bug 39955 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•