Closed Bug 17581 Opened 25 years ago Closed 25 years ago

Use ToNewCString and pass/return char* damage non ASCII data.

Categories

(MailNews Core :: MIME, defect, P3)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tao, Assigned: rhp)

Details

GOOD: http://lxr.mozilla.org/mozilla/source/mailnews/local/src/nsLocalStringBundle.cpp#53 Please review the below segment of code to see if we need to use ToNewUnicode() instead of ToCString() to retrieve the string buffer from nsAutoString. Using ToCString() might result in truncation of the second byte of a double-byte character. Using the Unicode variants is always safer than the CString one. Please reference the code segment above for the proper use. BAD: http://lxr.mozilla.org/mozilla/source/mailnews/mime/cthandlers/vcard/mimevcrd.cpp#1997 http://lxr.mozilla.org/mozilla/source/mailnews/mime/src/mimemoz2.cpp#1279 Thanks
QA Contact: lchiang → tao
Summary: [DOGFOOD] Please use ToNewUnicode() to retrieve string buffer → Please use ToNewUnicode() to retrieve string buffer
Please fix it by M12. Thanks. Remove [DOGFOOD] from summary.
Status: NEW → ASSIGNED
Target Milestone: M20
I just looked at this and I don't know how we can make this change without a substancial rewrite of libmime. If you change the return of char * to PRUnichar in these cases, then the caller needs to deal with it at PRUnichar * and so on, and so on.... I just can't see this happening any time soon. - rhp
QA Contact: tao → nhotta
Hi, Naoki: Per my discussion with Frank TAng, I 'll let you drive this issue. Thanks, Tao
libmime in SeaMonkey is a rewrite. I don't understand why we pass data as char* instead of PRUnichar* in the 1st rewrite. Everyone else is using nsString or PRUnichar* in interface already....
Summary: Please use ToNewUnicode() to retrieve string buffer → Use ToNewCString and pass/return char* damage non ASCII data.
Change "Please use ToNewUnicode() to retrieve string buffer" to "Use ToNewCString and pass/return char* damage non ASCII data."
libmime in SeaMonkey is NOT a rewrite. I would like to see where things are broken to see this bug "in action" instead of having an automated routine generate bugs for me. I (as do the rest of client engineering) have more bugs than I'll ever get to before we call it Dogfood/Beta/PR?/FCS or any other acronym you would like to think of. I APOLOGIZE IN ADVANCE FOR THE TONE OF THIS RESPONSE. I would just appreciate it if people would take my word for it when I say that a particular change is non-trivial. I've been responsible for libmime since 5.0 started so I know a thing or two about it by now. - rhp
I looked at the code with tao. I think they can use UTF-8 like other parts of libmime so unicode text can be passed without data loss through char*. Once those text needs to be passed to UI (or whatever needs Unichar*) they can be converted from UTF-8 to UCS-2 (PRUnichar*) using nsTextFormater. We want to test the two cases mentioned by tao and may create separate bugs which can be tested/verified easier. MimeGetStringByIDREAL, I cannot find callers of this is this going to be used in future? VCardGetStringByID, this is used for any localizable string for vCard, correct?
After M11 is done, I am going to turn on the real String Bundle calls. For a while, string bundles did not work no the necko thread, but that is fixed now. - rhp
Target Milestone: M20 → M11
Naoki made a really good point that my brain seemed to forget. We deal with UTF-8 for most of libmime. So, a conversion to UTF-8 is a good thing that will work with "char *", then we convert to UCS-2 before we do quoting, etc...but in the case of display only, we just have our friend the META tag set the charset to UTF-8. I have the real calls turned on in my tree with this fix and I hope to get it checked in today. - rhp
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
After the nice reminder from Naoki, this has been changed to use the UTF-8 output format. - rhp
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.