73403 - Extra tab character is inserted into a subject

Status: VERIFIED FIXED

(MailNews Core :: MIME, defect, P2)

Description

[Koike Kazuhiko]

Extra tab character is inserted into a subject if the subject contains
ASCII and Japanese characters, with brackets.

Steps to reproduce:
(1) Open a mail compose window.
(2) Enter a subject string which contains ASCII and Japanese characters,
with brackets. For example "<aiueo>abcdef(<kakikukeko>ghijkl)".
Enter <aiueo> and <kakikukeko> in Japanese.
(3) Save the message. It contains an extra tab character before <kakikukeko>.

Comment 1

[Koike Kazuhiko]

If a subject is encoded as a multiple line and the end of first line is
ASCII character, the tab character at the top of next line is treated as
a part of subject contents.

Subject: =?ISO-2022-JP?B?GyRCJCIkJBsoQmFiYxskQiQmJCgkKhsoQg==?="
	=?ISO-2022-JP?B?ZGVmGyRCJCskLSQvJDEkMxsoQg==?=:

Comment 2

[nhottanscp]

Reassign to jgmyers.

Comment 3

[John G. Myers]

Per RFC 822, the tab at the start of the next line is *supposed* to be part of 
the subject contents.  This is a bug in the encoder, not the decoder.

Comment 4

[Jun MUTO]

I think it's not only a encoder's bug but also a decoder's.
The HTAB between 'encoded-word's is ignored.

RFC 2047 "Message Header Extensions"
6.2. Display of 'encoded-word's :
   When displaying a particular header field that contains multiple
   'encoded-word's, any 'linear-white-space' that separates a pair of
   adjacent 'encoded-word's is ignored.  (This is to allow the use of
   multiple 'encoded-word's to represent long strings of unencoded text,
   without having to separate 'encoded-word's where spaces occur in the
   unencoded text.)


The "linear-white-space" is defined in RFC 822.

RFC 822 "Standard for ARPA Internet Text Messages"
3.2.  HEADER FIELD DEFINITIONS :
     LWSP-char   =  SPACE / HTAB                 ; semantics = SPACE

     linear-white-space =  1*([CRLF] LWSP-char)  ; semantics = SPACE
                                                 ; CRLF => folding

Comment 5

[John G. Myers]

The HTAB in question is not between encoded-words.  It is between a double quote 
<"> and an encoded-word.

Comment 6

[Koike Kazuhiko]

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

Comment 7

[Henrik Gemal]

all platforms...

Comment 8

[Henrik Gemal]

Attachment: testmail

Comment 9

[Henrik Gemal]

any update here. It's kind of imbaressing that we cant display a simple header
like subject correct...
You get small quares in the thread pane...

I still dont quite understand why we cant reuse code, since the subject is
showed correct in the message pane, but not in the thread pane...

I'm talking about a header like this:
Subject: =?iso-8859-1?Q?Informationsm=F8de?= vedr. CMD, samt 
	=?iso-8859-1?Q?fagpr=E6sentation?= d. 21.5


which has a square after "samt" in the thread pane.

build 20011014

Comment 10

[John G. Myers]

The inconsistent display of HTAB characters is bug 64948.  This bug covers the
issue of the encoder incorrectly inserting a semantically meaningful HTAB.

Unfortunately, I don't have time to work on Mozilla these days.

Comment 11

[ji]

Nominating for nsbeta1.

Comment 12

[Takayuki Tei]

Since no one is working on this, I take this bug.
Set TM to 0.9.7.

I'll most likely end up with re-writing whole encoder.

Comment 13

[Takayuki Tei]

Set TM to 0.9.8.

Comment 14

[Denis Antrushin]

I think that decoder should replace <CRLF><LWSP>+ with <SPACE> char (or even
<LWSP>*<CRLF><LWSP>+ with <SPACE>) and encoder should insert <CRLF> before 
existing <LWSP> char(s) (with no trailing whitespaces). 
This way we can handle rfc822-, rfc2822- and no_any_rfc-compliant header 
wrapping in reasonable way, while being rfc2822-compliant itself.

Am I missing something?

Comment 15

[John G. Myers]

Whitespace sometimes has semantic meaning.  Removing whitespace in certain
circumstances can change the meaning of the message and violates the standards.

Comment 16

[Denis Antrushin]

But rfc822 and rfc2822 quite different describes header folding:
rfc822: a CRLF immediately followed by AT LEAST one LWSP-char may instead  be
        inserted
rfc2822: a CRLF may be inserted before any WSP

How'd we handle this? Not all mailers are rfc822 conformant.
Also, may be I misunderstand this, but rfc2822 says (section 3.2.3):
   Runs of FWS, comment or CFWS that occur between lexical tokens in a
   structured field header are semantically interpreted as a single
   space character.
and 
   FWS =  ([*WSP CRLF] 1*WSP) /   ; Folding white space
Doesn't this phrase means that we can replace <CRLF><LWSP>+ with 
single <SPACE>?

Comment 17

[John G. Myers]

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

Comment 18

[Denis Antrushin]

see also bug 37088 

Comment 19

[Takayuki Tei]

Set TM to 0.9.9.

Comment 20

[Takayuki Tei]

Attachment: proposed patch

pls apply this patch to 20020128 or later source tree.
i want at least a couple of people to try this patch
before asking for review.

Comment 21

[nhottanscp]

+NS_MSG_BASE char      *nsMsgI18NEncodeMimePartIIStr(const char *header, PRBool
bStructured, const char *charset, PRBool bUseMime);

Because most of the callers set PR_TRUE for 'bStructured', how about make
PR_TRUE as the default? Also please add a comment about 'bStructured' in
nsMsgI18N.h.

Comment 22

[Takayuki Tei]

> Because most of the callers set PR_TRUE for 'bStructured', 
> how about make PR_TRUE as the default?

Thought about it, and didn't do so.  Developper should explicitly specify
which type of field it is to avoid mess in future.

> Also please add a comment about 'bStructured' in nsMsgI18N.h.

Will do.

Comment 23

[Takayuki Tei]

Attachment: proposed patch

updated based on nhotta's comment.

Comment 24

[Denis Antrushin]

This patch also fixes bug 37088, bug 92355 and bug 98675. Great!

As for patch itself, some nits for utf8_apply_rfc2047_encoding():

1) You'll be asked to replace nsCRT::strlen to strlen, (foo == NULL) to (!foo)
and, most likely, NULL to nsnull. I'm just have chance to say it ahead of
reviewrs :-)

2) you use strlen(src) twice - when computing finallen and in
lookfor_angleaddr(). You can just add srcLen param to lookfor_angleaddr().

3) while (*src) {
     ...
     sprintf(f, "=?%s?%c?", charset, encodingmethod);
     f += linelen = strlen(f);
     ....
  As far as charset is a constant, you can compute its length outside of loop
  and then f += linelen = charsetLen + 5 /* 5: =??B? */

4) f += strlen(f)
   You don't need to call strlen(f) most of time. Either you add const-lenght
   strings to it, or you know how much was added - intlmime_encode_(b|qp)
   knows lenght of converted text. B64 encoder already returns it. QP
   encoder always return 0, why not to return length of output string?

5) You can also move 
        !nsCRT::strcasecmp(charset, "us-ascii") ? "ISO-8859-1" : charset 
   out of loops.

6) Is it possible to get rid of goto statement by changing condition on
   line 607 from 'if (linelen + offset > foldlen)' to
   'if (!*src || linelen + offset > foldlen)' ?
   (or 'if ((!*src && linelen) || linelen + offset > foldlen)' )

7) Is nsMsgI18NSaveAsCharset() fast enough? It's called many times
   (as many as number of chars in src string). While it's not
   noticeable on amount of data, how fast it will work with very
   long / many haedars? Or it's not much concern?

Comment 25

[marina]

Denis, would the fix for this bug help # 64948?

Comment 26

[Denis Antrushin]

Nope, bug 64948 is about decoder; this patch only fixes encoder...

Comment 27

[Takayuki Tei]

The second patch posted yesterday contains conv_htab() which converts
TAB to 0x20 at the end of MIME header decoder.  In other words, the patch
fixes #64948 as well.

Comment 28

[Denis Antrushin]

Oops, sorry, overlooked that.
Then this patch also fixes 117901...

Comment 29

[Takayuki Tei]

I've found a bug in the latest patch.  Working on a new one right now.

Comment 30

[nhottanscp]

nsMsgI18NSaveAsCharset() is used because we need fallback when converting from
unicode (e.g. trademark symbol to TM). 
You can convert the input string in advance to know if we need a fallback or not
(by checking the error code NS_ERROR_UENC_NOMAPPING). Then you only need to call
nsMsgI18NSaveAsCharset() for that case. You can also know the converted length
so you can use it to estimate the encoded string size. 
If you do the per character conversion then it's better to create the converter
once in the code.

Comment 31

[Takayuki Tei]

Attachment: proposed patch

now handles multiple addresses correctly.

Comment 32

[Takayuki Tei]

Attachment: get rid of nsMsgI18NSaveAsCharset()

should performs much faster than previous version.
i think i've done whatever i can do for now.
please review the code and give me an approval to checkin.

Comment 33

[Koike Kazuhiko]

Attachment: Stack trace

attachment 67549 [details] [diff] [review] causes a crash when I try to save a draft message.

Comment 34

[Takayuki Tei]

The stack trace looks kinda bogus.  Can you build a debug bits and see what you
get?  On Win32, it works just fine.

Comment 35

[Takayuki Tei]

Attachment: consolidated patch for 73403 and 21267

verified that the patch works on top of 20020202 snapshot on Win32.

Comment 36

[Takayuki Tei]

duplicated the core dump on Red Hat 7.2.

Comment 37

[Takayuki Tei]

Attachment: proposed patch

fix Linux core bug. removed patch for bug 21267 per [e-mail]

Comment 38

[nhottanscp]

I am reviewing the patch.

Comment 39

[nhottanscp]

1) nsMsgI18NDecodeMimeHeader, I think the decoder change is not needed for this
bug, please remove.

2) 'fieldnamelen', why is this needed? What does it mead to pass 0 as
'fieldnamelen'?
+ * @param fieldnamelen [IN] Header field name length. (e.g. "From: " -> 6)

3) In intlmime_encode_q(), is using '_' new or also existed in the old code?
+    else if (*in == ' ') {
+      *out++ = '_';

4) In generate_encodedwords(), 
"char *src, char *charset," are those const?

the charset name for the header has to be created later because it may change later
+  sprintf(encodedword_head, "=?%s?%c?", charset, method);

5) The Alias code, there is a new way of doing it by using
nsICharsetConverterManager2.

  nsCOMPtr <nsICharsetConverterManager2> ccm2 =
do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv);

  nsCOMPtr <nsIAtom> charsetAtom;
  rv = ccm2->GetCharsetAtom(NS_ConvertASCIItoUCS2(charset).get(),
getter_AddRefs(charsetAtom));

  const PRUnichar *charsetName;
  charsetAtom->GetUnicode(&charsetName);

Now you can convert to char* and use it to pass to nsISaveAsCharset (instead of
ToCString), also use this charset name for the header.
NS_ConvertUCS2toUTF8((PRUnichar *)charsetName).get();

6) This is for plain text conversion, so 'attr_EntityNone' instead of
'attr_EntityAfterCharsetConv'.
+    rv = aConv->Init(aCharset.ToCString(charset_buf, kMAX_CSNAME+1), 
+                        nsISaveAsCharset::attr_FallbackQuestionMark +
nsISaveAsCharset::attr_EntityAfterCharsetConv, 
+                        nsIEntityConverter::transliterate);

7) Please move the pref service call inside the "if (conv_kana < 0)"

+    nsCOMPtr<nsIPref> prefs(do_GetService(kPrefCID, &rv)); 

8) The loop, can you loop after convert to UCS2 (and transform)? Then you don't
need to call utf8_nextchar (can do ustr++) also only need to call
NS_ConvertUTF8toUCS2 and textTransform once.

+    while (*src && (olen < outlen)) {
+      src = (char *)utf8_nextchar((unsigned char *)src);
+      convlen = src - linehead;
+      tmp = *(linehead+convlen);
+      *(linehead+convlen) = '\0';
+      // UTF-8 to mail charset conversion
+      inString = NS_ConvertUTF8toUCS2(linehead, convlen).get();
+      if (textTransform) {
+        nsString aText(inString);
+        rv = textTransform->Change(aText, aResult);

Comment 40

[Takayuki Tei]

Attachment: revised patch per [e-mail] 's review

> 1) nsMsgI18NDecodeMimeHeader, I think the decoder change is not needed for
this
> bug, please remove.

fixed.


> 2) 'fieldnamelen', why is this needed? What does it mead to pass 0 as
> 'fieldnamelen'?
> + * @param fieldnamelen [IN] Header field name length. (e.g. "From: " -> 6)

To satisfy "2.1.1. Line Length Limits" of RFC-2822.


> 3) In intlmime_encode_q(), is using '_' new or also existed in the old code?
> +    else if (*in == ' ') {
> +	 *out++ = '_';

It's new.  As recommended in '4.2. The "Q" encoding' (2) of RFC-2047.


> 4) In generate_encodedwords(), 
> "char *src, char *charset," are those const?
> 
> the charset name for the header has to be created later because it may change
later
> +  sprintf(encodedword_head, "=?%s?%c?", charset, method);

fixed.


> 5) The Alias code, there is a new way of doing it by using
> nsICharsetConverterManager2.

done.


> 6) This is for plain text conversion, so 'attr_EntityNone' instead of
> 'attr_EntityAfterCharsetConv'.

fixed.


> 7) Please move the pref service call inside the "if (conv_kana < 0)"

fixed.


> 8) The loop, can you loop after convert to UCS2 (and transform)? Then you
don't
> need to call utf8_nextchar (can do ustr++) also only need to call
> NS_ConvertUTF8toUCS2 and textTransform once.

fixed.

Comment 41

[nhottanscp]

Comment on attachment 68181 [details] [diff] [review]
revised patch per [e-mail] 's review

r=nhotta

According to the document,
http://lxr.mozilla.org/seamonkey/source/string/doc/string-guide.html
holding the pointer is not good.

+  // Let's deal with UCS2 string
+  pSrc = (PRUnichar *)NS_ConvertUTF8toUCS2(src, strlen(src)).get();

I think you can do 
NS_ConvertUTF8toUCS2 ucs2(src, len);
then access the string by ucs2.get() when needed.

There may be a better way, ask the super reviwer.

Comment 42

[nhottanscp]

cc to ducarroz, in case he wants to review the patch.

Comment 43

[Takayuki Tei]

Attachment: no longer holds pointer & code clean up

successfully built on Win32 and Linux.

Comment 44

[Jean-Francois Ducarroz]

I'll let nhotta doing the review as I am currently kind of overflown with code
review...

Comment 45

[Takayuki Tei]

so, can i check this in?

Comment 46

[John G. Myers]

Taka: you need to get nhotta to review your revised patch, then you need to get
a super-reviewer to super-review your patch.

Comment 47

[Takayuki Tei]

I've already sent e-mail requests to nhotta and super-reviewers last week.

Comment 48

[nhottanscp]

+    // Deal with UCS2 pointer
+    {
+      NS_ConvertUTF8toUCS2 ucs2(pUTF8, strlen(pUTF8));
+      size_t bytes = sizeof(PRUnichar) * (ucs2.Length() + 1);
+      pUCS2 = _pUCS2 = (PRUnichar *)PR_Malloc(bytes);
+      if (!pUCS2)
+        return -1;
+      memcpy(pUCS2, PromiseFlatString(ucs2).get(), bytes);
+    }

Do you really need to duplicate the string?
Can you make the loop section as a separate function?
Or does something like this compile?
PromiseFlatString ucs2(NS_ConvertUTF8toUCS2(pUTF8, strlen(pUTF8)));
You can use nsCRT:strdup() if you need to duplicate.

Comment 49

[Scott MacGregor]

Comment on attachment 68652 [details] [diff] [review]
no longer holds pointer & code clean up

Some comments:

1) While we don't have an enforced standard for variable declarations in
methods, we don't use the system where you put a 'b' before booleans and the
like. However we do try to recommend using an 'a' for argument. i.e. aUseMime
and aStructured.

2) why do we want to make static char declarations for strings like
"Dispositiion-Notification-To: and "Return-Receipt-To"? Just use the string
literals directly in the line of code. Same for X-Template, From, Reply-to,
Organization, To, cc, subject, etc.  

3) do methods like GetTemplateName return allocated strings? If so, pStr is
leaking them. If not, can pStr be listed as a const char *? 

4)you use a lot of raw int types in generate_encodedwords. Shouldn't those be
PRInt32s or PRUint32s? 

5) In that same routine, I was confused by what we are doing to the charset
variable. We are given an ascii charset, we convert it to unicode and get an
atom for the charset. Then we get the unicode string form the atom and convert
that back to utf8. Doesn't that effectively give us the same charset string we
started with before all the conversions?

6) instead of using a static CID in that same routine for the pref service, use
the contract ID for the pref service. 

7) nsString aText(pUCS2);
that's a bad pattern to use becomes folks think an 'a' in front of a variable
name means it is an argument. Same with the aResult nsString in that routine  

8) We take a unichar pointer (pUCS2), copy it into a nsString just so we can
call TextTransform which then copies it into another nsString? That seems like
a lot of string copying to me on the surface. Why does the Change method
require the input to be an nsString and not a const PRUnichar *? Is it going to
manipulate the passed in argument? 

9) Is there a contract ID we can use instead of SaveAsCharsetCID? 

10) your new arguments in nsMimeConverter.h for fieldNameLen need to be
PRInt32s and not naked "int" types. 

I have to spend more time looking at the last half of the patch. There's a lot
of stuff in there. More comments to come later.

Comment 50

[nhottanscp]

Some of the changes are copied from my old code (not taka's fault).
8) I think nsITextTransform::Change needs to change to take a const PRUnichar*.
I don't think input needs to be modified. I will file a bug.

9) In nsISaveAsCharset.idl,
#define NS_SAVEASCHARSET_CONTRACTID "@mozilla.org/intl/saveascharset;1"

Comment 51

[nhottanscp]

8) filed a bug 125077

Comment 52

[Takayuki Tei]

[e-mail] wrote:

> Do you really need to duplicate the string?

Code tells that a pointer to PRUnichar needed.  Unless a pointer returned by
PromiseFlatString.get() is a flat, null teriminated PRUnichar string for the
lifetime of generate_encodedwords(), duplicating PromiseFlatString.get() string
is the safest way, as far as I can tell from the doc.

> Can you make the loop section as a separate function?

No.

Comment 53

[nhottanscp]

5)
Currently, intl/uconv takes charset as either nsIAtom or nsString. When we
create the charset atom, a charset name canonicalization is applied, so we need
that part. We need the UCS2 to UTF-8 conversion because nsIAtom only supports
wstring.
Usually, after gettomg a charset atom, there is no need to get the name since
uconv methods take a charset atom (e.g. to create a converter). The taka's code
needs a charset name in char* because it has to put the name into the message
header.

Comment 54

[nhottanscp]

A couple of additional points.

* Related to my last comment about 'charset'.

+    charset = (char *)NS_ConvertUCS2toUTF8((PRUnichar *)charsetName).get();
This has the same problem as the header string.

+    sprintf(encodedword_head, "=?%s?%c?", charset, method);
can change to 
sprintf(encodedword_head, "=?%s?%c?", 
        NS_ConvertUCS2toUTF8((PRUnichar)charsetName).get(), method);

+    if (!nsCRT::strcasecmp("ISO-2022-JP", charset)) {
can change to (no casecmp needed after the canonicalization)
if (!nsCRT::strcmp(NS_LITERAL_STING("ISO-2022-JP").get, charsetName)) {

* Does the code below leak if pUCS2 is already set?

+          nsString aText(pUCS2);
+          rv = textTransform->Change(aText, aResult);
+          if (NS_SUCCEEDED(rv))
+            pUCS2 = (PRUnichar *)aResult.get();
+        }

Comment 55

[Takayuki Tei]

Attachment: revised patch

Comment 56

[nhottanscp]

+        static NS_DEFINE_CID(kPrefCID, NS_PREF_CID);
+        nsCOMPtr<nsIPref> prefs(do_GetService(kPrefCID, &rv));

Please change to,
nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv)); 

+    static NS_DEFINE_CID(kSaveAsCharsetCID, NS_SAVEASCHARSET_CID);
+    rv = nsComponentManager::CreateInstance(kSaveAsCharsetCID, NULL, 
+                                            NS_GET_IID(nsISaveAsCharset),
getter_AddRefs(conv));

Please cahnge to,
conv = do_CreateInstance(NS_SAVEASCHARSET_CONTRACTID, &rv);

About the charset issue (comment #54), I prefer to use the NS_LITERAL_STRING for
the comparison, but it's up to you.

Comment 57

[Takayuki Tei]

Attachment: revised patch

Comment 58

[nhottanscp]

+            size_t bytes = sizeof(PRUnichar) * (result.Length() + 1);
+            pUCS2 = _pUCS2 = (PRUnichar *)PR_Malloc(bytes);
+            if (!pUCS2)
+              return -1;
+            memcpy(pUCS2, PromiseFlatString(result).get(), bytes);

Please change to,

pUCS2 = _pUCS2 = ToNewUnicode(result)
if (!pUCS2)
  return -1;

then r=nhotta.

Comment 59

[Takayuki Tei]

Attachment: revised patch. made a requested change.

Comment 60

[Takayuki Tei]

change component from "I18N" to "MIME".

Comment 61

[nhottanscp]

Comment on attachment 69322 [details] [diff] [review]
revised patch.  made a requested change.

r=nhotta

Comment 62

[Takayuki Tei]

Attachment: adopt new method introduced in bug 125077

Comment 63

[Scott MacGregor]

Comment on attachment 69590 [details] [diff] [review]
adopt new method introduced in bug 125077

1) You still haven't removed the 'b' prefix from all the arguments to the
methods I mentioned above. Please do that. If you want to use a letter use 'a'
for argument. That's the standard we've been using in mozilla. For examples
look at any .idl file in xpcom or mailnews\base\public. 

Example IDL here shows the useage too:http://www.mozilla.org/docs/modunote.htm

2) You didn't answer my question about methods like GetTemplatesName returning
an allocated string or not. Can you please look at the implemenation and verify
that they don't allocate new strings? (I don't believe they do). I want to make
sure we aren't leaking strings here.

3) Instead of using all those inline static chars so you can call sizeof, use
#defines at the top of the file

#define SUBJECT "Subject: "
then in your macro
ENCODE_AND_PUSH(subject, nsCRT::strlen(subject), PR_FALSE, (char *)pSubject,
charset, usemime);

Or if you don't want to make the strlen call add another define:
SUBJECT_LEN 8

In each call to the macro you are casting away the constness of the string.
That smells fishy. Why are you doing that? 

4) You said Naoki showed you how to do my comment #5 above about the string
conversions with the charsets. We need to ask him if that's the best way. It
sure looked like a ton of string coyping to go from an asci charset string back
to an ascii charset string again.


5) You are still using raw integers instead of PRInts in routines like
nsIMimeConverter.h

6) In intlmime_encode_b you are using strlen and strcpy. I think you want to be
using nsCRT::strlen  which is our all platform friendly versions.

7) In generate_encodedwords you use spritnf.  You should use the NSPR version:
PR_sprintf. Same routine you have calls to strlen which should be
nsCRT::strlen. 

8)minor nit but destruct_addresslist should probably be "destroy". destruct is
the wrong form of the word. You don't destruct a list, you destroy it. =)

9)some raw calls to strdup should be nsCRT::strdup after the code "else if (*s
== '<' || *s == '>')"

10) naked strlen:
outputlen = strlen(src) * 4 + kMAX_CSNAME + 8;

11) Use PR_sptrintf:
sprintf(outputtail, "\r\n %s", list->addrspec);











Can you reply to my comments here in the bug instead of in email so I can
better keep track of things when I go to look at the patch again? thanks!

Comment 64

[nhottanscp]

5)
uconv uses nsIAtom for charset, it has a name in wstring. In mail/news code,
char* is usually used for charset name because more code is based on char*
(libmime, msgsend) and charset name is in fact ASCII only. So there is a generic
issue of converting charset name between char* and PRUnichar*. I do not have a
good solution for that.
We can usually localize the conversions at the code getting uncode converters.
In this patch, he needs to create a message header which is sent out as char*.

Comment 65

[Takayuki Tei]

Attachment: revised patch

1) done.

2) doesn't return allocated memory.

3) use literal now. no more casting.

4) Naoki answered to this.

5) done.

6) done.

7) and 11)
>pwd
/src/mozilla.212/mozilla/nsprpub

>find . -name \*.h | xargs grep "sprintf.*("
./pr/include/md/sunos4.h:extern char *vsprintf(char *, const char *, va_list);
./pr/include/prprf.h:NSPR_API(char*) PR_sprintf_append(char *last, const char
*fmt, ...);
./pr/include/prprf.h:NSPR_API(char*) PR_vsprintf_append(char *last, const char
*fmt, va_list ap);

>

8) as you wish.

9) done.

10) done.

Comment 66

[Scott MacGregor]

The method in question is two lines above the code you pasted in yoru greP

nsprpub\pr\include\prprf.h:

/*
** sprintf into a PR_MALLOC'd buffer. Return a pointer to the malloc'd
** buffer on success, NULL on failure. Call "PR_smprintf_free" to release
** the memory returned.
*/
NSPR_API(char*) PR_smprintf(const char *fmt, ...);

Comment 67

[Takayuki Tei]

PR_smprintf() is not a mirrored copy of spirntf().  It allocates memory,
and sprintf into it.  Using PR_smprintf() means rewriting a part of whole logic.

Comment 68

[Scott MacGregor]

I meant to paste the routine right next to that one:
/*
** sprintf into a fixed size buffer. Guarantees that a NUL is at the end
** of the buffer. Returns the length of the written output, NOT including
** the NUL, or (PRUint32)-1 if an error occurs.
*/

NSPR_API(PRUint32) PR_snprintf(char *out, PRUint32 outlen, const char *fmt, ...);

Comment 69

[Takayuki Tei]

Attachment: replace sprintf with PR_snprintf

Comment 70

[Scott MacGregor]

Comment on attachment 70177 [details] [diff] [review]
replace sprintf with PR_snprintf

Couple comments, 
I still see a lot of casting going on in calls to ENCPDE_AND_PUSH like:
ENCODE_AND_PUSH("To: ", PR_TRUE, (char *)pTo, charset, usemime);

why do we need to cast pTo to a char *? What is it declared as that we need to
cast it?

I think I finally groked the charset stuff that you were trying to expain to me
Naoki. Thanks for your patience.

Why are we casting in the following lines?
strcpy(_charset, (char *)NS_ConvertUCS2toUTF8((PRUnichar *)charsetName).get());

NS_ConvertUCS2toUTF8().get9) is already going to give you a char ptr, why do we
cast the result again to a char *? charsetName is already defined as a
PRUnichar ptr, why do we cast it as a PRUnichar ptr?
(PRUnichar *)charsetName

That pattern is repeated a couple times. 

i.e.
charset = !nsCRT::strcasecmp(charset, "us-ascii") ? (char *)"ISO-8859-1" :
charset;
why are we casting a const char * literal?

Comment 71

[nhottanscp]

>Why are we casting in the following lines?
>strcpy(_charset, (char *)NS_ConvertUCS2toUTF8((PRUnichar *)charsetName).get());
charsetName is const PRUnichar*, so it needs the cast
NS_ConvertUCS2toUTF8.get() returns const char*, so it needs the cast

>charset = !nsCRT::strcasecmp(charset, "us-ascii") ? (char *)"ISO-8859-1" :
This case charset is char*, not const char*, so it needs the cast

My comment #54 mentioned about avoiding the casting. So it can use PRUnichar* to
compare like this. 
if (!nsCRT::strcmp(NS_LITERAL_STING("ISO-8859-1").get, charsetName))
This way, once it gets charset atom (and its name in const PRUnichar*), no need
to convert it back to char* except for special cases (e.g. write out the charset
name for MIME header).

Comment 72

[Takayuki Tei]

Attachment: revised patch - yet another (char *) hunting

>why do we need to cast pTo to a char *?

My bad.

Comment 73

[Scott MacGregor]

>charset = !nsCRT::strcasecmp(charset, "us-ascii") ? (char *)"ISO-8859-1" :
This case charset is char*, not const char*, so it needs the cast

should charset be changed to be a const char * then? I'm worried about casting a
literal string into a char str ptr. Is someone going to try to delete the memory
in charset? Are we going to try to manipulate it some how? who allocated the
"us-ascii" text that charset points to before we cast a literal into it? Are we
leaking the original string then?

Oh it looks like Taka addressed these issues directly in the last patch so we
don't do anymore unnecessary casting. Thanks for doing that! Although I'm still
worried about my question above as far as who deletes the text in the charset
variable if anyone. 

Comment 74

[Takayuki Tei]

charset is now const char *.

Comment 75

[Scott MacGregor]

Comment on attachment 70388 [details] [diff] [review]
revised patch - yet another (char *) hunting

sr=mscott

Comment 76

[Takayuki Tei]

Following files have been updated and checked into trunk:

    mailnews/base/util/nsMsgI18N.cpp
    mailnews/base/util/nsMsgI18N.h
    mailnews/compose/src/nsMsgCompUtils.cpp
    mailnews/compose/src/nsMsgSend.cpp
    mailnews/mime/public/nsIMimeConverter.h
    mailnews/mime/src/comi18n.cpp
    mailnews/mime/src/comi18n.h
    mailnews/mime/src/mimehdrs.cpp
    mailnews/mime/src/mimehdrs.h
    mailnews/mime/src/nsMimeConverter.cpp
    mailnews/mime/src/nsMimeConverter.h

Mark as fixed.

Comment 77

[nhottanscp]

I was looking the code for other bug and found that the macro always takes a
literal string (e.g. "Subject: "). So it can use sizeof or it can take a string
length instead of a string.

223 #define ENCODE_AND_PUSH(name, structured, body, charset, usemime) \
224 { \
225 PUSH_STRING((name)); \
226 convbuf = nsMsgI18NEncodeMimePartIIStr((body), (structured), (charset),
nsCRT::strlen(name), (usemime)); \
227 if (convbuf) { \
228 PUSH_STRING (convbuf); \
229 PR_FREEIF(convbuf); \
230 } \
231 else \
232 PUSH_STRING((body)); \
233 PUSH_NEWLINE (); \
234 }

Comment 78

[ji]

QA contact to kasumi. Thanks.

Comment 79

[Kasumi]

tested on 2002-06-26-08-1.0
Windows XP Pro. JA
confirmed to fix.