92355 - Forward/Reply from/to newsgroup:comma and space are treated incorrectly in headers

Status: VERIFIED FIXED

(MailNews Core :: Internationalization, defect, P3)

Description

[marina]

*** observed with 2001-07-25-0.9.2 build ****
i've seen it so far in non-latin1 newsgroups ( mainly russian that uses commas
in their headers), it diesn't happen with a german message or english one
Steps to reproduce:
- go to this newsgroup on news.mcom.com server : kharkov.business ( this is a
group that advertise things for sale so comma in the header is almost in every
message);
- select a message with comma in the header;
- forward message to your account;
- get the message and note:
 the header in the message body ( the only place it happens) is parsed
incorrectly , commas are inside the word and the actual header doesn't make any
sense.
note that it doesn't happen if you would copy the same message to the IMAP or
POP folder, screen shot to follow

Comment 1

[marina]

Attachment: this is the correct display of the header in the body

Comment 2

[marina]

Attachment: here is the header where the first comma is inside the word and the second one is ommited

Comment 3

[nhottanscp]

Marina, could you attach a screen shot of the same message in IMAP or local 
message, so we can see what is the expected result?

Comment 4

[marina]

Naoki, the first screen shot i attached is the expected result

Comment 5

[nhottanscp]

Okay, sorry about that.

Comment 6

[marina]

Attachment: the header exibits the problem and the text in the message body is correct so could be used for testing purposes

Comment 7

[nhottanscp]

cc to taka, could be a dup of the MIME encoder bug

Comment 8

[Denis Antrushin]

I found the reason of this bug and have fix in hand.
I'll send it after some testing (along with problem description).

I think it'll fix bug 98675 as well

Comment 9

[Denis Antrushin]

OK, here's description of the problem:
When we encode mime header we break it in word at mail separator chars
(by replacing char at separator's position with \0 and and storing separator).
After we endoded this word, we append separator to encoded buffer and continue
to next word.
But sometimes word to encode is too line to fit into line, so we encode only
part of word and continue. Here's problem arises. Even if we had cut word, 
we still append separator char to encoded buffer and not restoring it in
source buffer. This is wrong, instead we should just put separator back
into source string and continue break it into words from last processed 
position. Thus, fix would be:

       while ( PR_TRUE ) {
         int iBufLen;    /* converted buffer's length, not BASE64 */
         if ( len > iThreshold )
+        {
           len = ResetLen(iThreshold, begin);
+          if (sep_p) {
+              *sep_p = sep; /*restore the original character */
+              sep = '\0';   /*to be processed with the rest  */
+              sep_p = NULL; /*of this chunk in next iteration*/
+          }
+        }

I'll attach full patch in a minute

Comment 10

[Denis Antrushin]

Attachment: patch, v1

I also made some other changes, for example
moved nsCRT::strlen(charset) outside of loop and compute it only once;
compute nsCRT::strcasecmp(intl_message_header_encoding((const char *) charset),
kMsgHeaderBEncoding) only once as well.

I also would like to make some other (cosmetic) changes, but these not related
to bug itself:
1. Add outStrLen parameter to intlmime_encode_base64_buf and
intlmime_encode_qp_buf, so we will not need to do strlen on encoded buffer.
2. Replace PL_strcat of short literal strings:
PL_strcat(buf1, "?=") to *buf1++ = '?'; *buf1++ = '=';
3. replace while(*p == ' ') p++; with while(*p++ == ' ');
4. replace expression like 
if (non_ascii)
  return p;
else
  return NULL;

with return (non_ascii) ? p : NULL;


Patch tested with testcases from this bug, bug 98675 (japanese subjects) and
smoketest mail folder.

Comment 11

[nhottanscp]

I think taka is working on rewriting the encoder, but it's fine to improve the
current code. 
Denis, could you try testing cases of the bugs assigned to taka (bug 73403, bug
104447, bug 102610) with your patch?

About the patch, now the threshold calculation is different ('+7' vs '-7'). Is
this intended? Need a comment about this.

+  mime2CharsLen = nsCRT::strlen(charset) + 7;
 
-  default_iThreshold = iEffectLen = ( maxLineLen - nsCRT::strlen(charset) - 7 )
* 3 / 4;
+  default_iThreshold = iEffectLen = ( maxLineLen - mime2CharsLen ) * 3 / 4;

Comment 12

[nhottanscp]

My mistake, never mind about my comment about the threshold.

Comment 13

[Denis Antrushin]

Ooops, proposed fix for 104447 contains fix for this problem (althought it's about
long headers folding)....
Bug 102610 doesn't has testcases, only screenshots and
bug 73403 is about folding as well, so this fix will not help there...

Comment 14

[nhottanscp]

Comment on attachment 65606 [details] [diff] [review]
patch, v1

r=nhotta

Thanks for fixing this.

Comment 15

[(not reading, please use [e-mail] instead)]

Comment on attachment 65606 [details] [diff] [review]
patch, v1

some nits:

>   int default_iThreshold;
>   int iThreshold;         /* how many bytes we can convert from the src */
>   int iEffectLen;         /* the maximum length we can convert from the src */
>+  int mime2CharsLen;      /* length of charset name plus =? ?B? ?= chars  */
>   PRBool bChop = PR_FALSE;
>+  PRBool bBase64Encode = PR_FALSE;

shouldn't those ints be PRInt32?

>   if (src == NULL)
>     return NULL;
>@@ -625,10 +624,13 @@
>   if (srcbuf == NULL)
>     return NULL;
>   begin = srcbuf;

instead of "if (foo == NULL)", that should be

if (!foo)

and note, I think you want nsnull instead of NULL;

>+  mime2CharsLen = nsCRT::strlen(charset) + 7;

use strlen(), it is going to be faster than nsCRT::strlen() and PL_strlen().

>+  bBase64Encode = !nsCRT::strcasecmp(intl_message_header_encoding((const char *) charset), kMsgHeaderBEncoding);

do charset.get() instead of casting.


>+              sep_p = NULL; /*of this chunk in next iteration*/

again, I think you want nsnull to be consistent.

>+         mime2CharsLen: =?<charset>?B?..?=, 3: CR LF TAB */
>+      convlen = nsCRT::strlen(buf2) + mime2CharsLen;

strlen(), not nsCRT::strlen()

fix those, and sr=sspitzer

Comment 16

[Denis Antrushin]

Attachment: patch, v2

De-nitted! (denitized?) :-)
NULL -> nsnull
nsCRT::strlen() -> strlen()
int -> PRInt32
foo == NULL -> !foo

Cannot do charset.get(), because it's char*, not nsXXXCString, sorry ;-)

Comment 17

[nhottanscp]

checked in

Thank you for the contribution.

Comment 18

[marina]

it does work correctly now, commas are there where they supposed to be, but when
are we going to fix a space problem that shows up as a square in the thread???