68993 - Can't send attachment with filename containing Shift-JIS trail byte 7C

Status: VERIFIED FIXED

(MailNews Core :: Internationalization, defect)

Description

[Wesley George]

<summary>
Can't send attachment with filename containing Unicode 7AF9 code point

<repro environment>
build 02-15-06 + Win98-JA

not repro on:
IE5.5 + Win98-JA

<repro steps>
1. save a file containing the Unicode code point value 7AF9 (The Japanese
character "take", takenoko no take)
2. try to send the file as an attachment (optional step: set the character
coding to Japanese ISO-2022-JP before sending message) 

<result>
Alert message "Sending of the message failed".

Comment 1

[Wesley George]

Attachment: Sample attachment file

Comment 2

[Wesley George]

not repro on:
build 02-14-08 + Mac OS 9.04-JA

Comment 3

[nhottanscp]

Is that specific to \u7AF9 only?

Comment 4

[ji]

"take" in ShiftJis has the trail byte same as ascii "|". 

Comment 5

[ji]

Another character which has the same trail byte: ‘| (souji no sou) has this problem as well.

Comment 6

[Wesley George]

Changed Summary to "Can't send attachment with filename containing Shift-JIS
trail byte 7C"

Comment 7

[nhottanscp]

How about the "Attachments:" area in the compose window?
Is the file name shown correctly in there?

Comment 8

[ji]

Yes, the Japanese attachment filename showing correctly on the compose window.

Comment 9

[ji]

Checked Mac build. On Mac, the mail with the attachment can be sent out,
but when received, the attachment filename containing this kind of problematic
chars is not displayed correctly on the envelope, for example, filename of
"take" is displayed as "%92|"

Comment 10

[nhottanscp]

On Macintosh, the file name is not MIME encoded at all. That's not specific to 
"TAKE" character. I used MacOS9 JA. We need to see if this is Mac specific.

Xianglan, could you send two messages to nhotta from Windows JA? One with file 
name "TAKE" and other with a file name without 0x7C?

Comment 11

[ji]

Naoki, I just sent you a mail with an attachment w/o "0x7c" in the filename.
Sending a mail with an attachment filename containing "0x7c" always fails on
Windows.

Comment 12

[ji]

>On Macintosh, the file name is not MIME encoded at all. That's not >specific to
"TAKE" character. I used MacOS9 JA. We need to see if this >is Mac specific.

Yes. On Mac, whether the filename is MIME encoded or not depends on the filename
length. If the Ja filename contains 4 or less Japanese chars, the filename won't
get MIME encoded. If the filenames contains more chars, it gets MIME encoded,
but not correctly encoded if it is sent from an English Mac OS. So the only
working condition is to send the mail from a Japanese MAC OS and the filename
must contains 5 or more Ja chars. I'll file a seperate bug for this.

Comment 13

[ji]

And the problem I mentioned above is Mac specific.

Comment 14

[ji]

On Ja Mac, the "0x7c" char doesn't cause the problem as it does on Windows. I
used the filename containing more than 5 ja chars and one of chars is "0x7c"
char, sending doesn't fail and filename shows correctly on the envelope. So it
looks like the "0x7c" problem is win32 specific.

Comment 15

[ji]

Bug 70341 has been filed for the Mac specific problem.

Comment 16

[nhottanscp]

nsMsgComposeAndSend::AddCompFieldLocalAttachments in nsMsgSend.cpp, it replaces 
'|' by ':'.
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsMsgSend.cpp#1854

1854 #ifdef XP_WIN 
1855 str.ReplaceChar('|', ':');
1856 #endif

Cc to ducarroz, do you know why this is needed?

Comment 17

[Charles Manske]

The "|" character appears when you convert a Windows filespec into a URL,
e.g.:
C:\netscape\myfile.html
becomes
C|/netscape/myfile.html
I don't pressume to know this code, but why are we manipulating char* here
and not Unicode strings? We have similar native-file to URL conversions in
Composer in File Save methods where the use of Unicode prevents this kind of
problem.

Comment 18

[nhottanscp]

I agree using unicode is better, that would be a separate project.
Since the code is in #ifdef WIN, I am going to use ::IsDBCSLeadByte to avoid
replacing DBCS characters.

Comment 19

[Jean-Francois Ducarroz]

we should no hack the path manually if we want to make a url with it, we have
API for that!

Comment 20

[nhottanscp]

So can we replace the line
1855 str.ReplaceChar('|', ':');
with that API? Where is that API?

Also we can change to replace a substring "|\" instead of a character '|'.

Comment 21

[Jean-Francois Ducarroz]

you should be able to do something like that:

nsCOMPtr<nsILocalFile> localFile = do_CreateInstance(NS_LOCAL_FILE_CONTACTID, &rv);
localFile->InitWithPath(filePath) or InitWithUnicodePath
char* url;
localFile->GetURL(url);

Comment 22

[nhottanscp]

nsMsgAttachmentHandler is written using nsFileSpec and the string "str" is used
for other things. Also I am not sure if nsMsgNewURL() is exactly equivalent to
nsILocalFile::GetURI().
For this bug, I am going with replacing "|\".

1875         nsMsgNewURL(&(m_attachments[newLoc].mURL), str);
1876
1877         if (m_attachments[newLoc].mFileSpec)
1878           delete (m_attachments[newLoc].mFileSpec);
1879         m_attachments[newLoc].mFileSpec = new nsFileSpec( nsFileURL((const
char *) str) );
1880
1881         if (m_attachments[newLoc].mURL)
1882           msg_pick_real_name(&m_attachments[newLoc],
mCompFields->GetCharacterSet());
1883
1884         // Now, most importantly, we need to figure out what the content
type is for
1885         // this attachment...If we can't, then just make it
application/octet-stream
1886         nsresult  rv = NS_OK;
1887         nsCOMPtr<nsIMIMEService> mimeFinder
(do_GetService(NS_MIMESERVICE_CONTRACTID, &rv));
1888         if (NS_SUCCEEDED(rv) && mimeFinder)
1889         {
1890           char *fileExt =
nsMsgGetExtensionFromFileURL(NS_ConvertASCIItoUCS2(str));
1891           if (fileExt)
1892             mimeFinder->GetTypeFromExtension(fileExt,
&(m_attachments[newLoc].m_type));
1893
1894           PR_FREEIF(fileExt);
1895         }

Comment 23

[nhottanscp]

Attachment: Patch: replace a substring "|/" instead of a character '|'.

Comment 24

[nhottanscp]

As long as I seen in the debugger, the string is already ":/" instead of "|/".
The change was made in 09/1999 it may be not be needed anymore.
I also do not understand why it's changing from URL to native windows style.
Anyway, I don't intend to rewrite the attachment code for this bug.
Jean-Francois, please review.

Comment 25

[Jean-Francois Ducarroz]

R=ducarroz

Comment 26

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

replacing | with : broke shift-jis

will replacing |/ with :/ break any encoding?

is there a way to fix this without using such a hack?  if that is beyond the
scope of this bug, we should at least open a new bug on that (and probably
assign it to ducarroz).

Comment 27

[Jean-Francois Ducarroz]

why not just totally remove this hack! If you are still able to send file on
window, I will vote for it...

Comment 28

[nhottanscp]

Attachment: New patch: removed the line to replace '|' with ':' since that's already replaced in the string

Comment 29

[nhottanscp]

Jean-Francois, please review the new patch, send attachment and send webpage 
still works without that line.

Comment 30

[Jean-Francois Ducarroz]

great, R=ducarroz

Comment 31

[nhottanscp]

Filed a bug 71535 for nsFileSpec issue.

Comment 32

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

sr=sspitzer

Comment 33

[nhottanscp]

checked in

Comment 34

[ji]

Verified with 04/03 win32 build. It is fixed.

Comment 35

[ji]

It's impossible to send out this kind of attachments with no or other file 
extensions, like .doc. Reopened the bug.

Comment 36

[Frank Tang]

give nhotta to investiate this issue. 

Comment 37

[nhottanscp]

There is an error when try to create a temp file.
For .html and .txt, name of the temp file is something like "nsmail.tmp". But
other files, the original file name is used. The error happens in NSPR called by
nsILocalFile. I think NSPR does not recognize multi-byte file names.
It might be possible to try another name after the failure. 

Comment 38

[nhottanscp]

Attachment: Patch, in SnarfAttachment do not use the original file name if it contains '|', "nsmail-X.tmp" to be used instead.

Comment 39

[nhottanscp]

Attachment: Patch, ignore the last patch, I attached a wrong one.

Comment 40

[nhottanscp]

I asked varada to review the patch.

IQA, is 0x7c the only case it fails? Please try other characters (e.g.
characters from 0x8940 to 0x897E). I think easy way is to create four files each
has 15 characters (e.g. 0x8940 - 0x894F), so you don't need to try 60 different
cases.

Comment 41

[varada]

Windows will fail with <>/\:*?"| and not just |

Comment 42

[nhottanscp]

They are under 0x40 (except 0x5c and 0x7c) not overwrap with Shift_JIS charset
which is used for JA windows file system.
I tried a case 0x5c but did not have the problem.

Comment 43

[nhottanscp]

cc to [e-mail], please see my last comment

Comment 44

[ji]

I tried the filename containing characters in 0x8940 to 0x897E, only 897c 
character causes this problem.

Comment 45

[varada]

The patch is good; r=varada

Comment 46

[Jean-Francois Ducarroz]

R=ducarroz

Comment 47

[David :Bienvenu]

sr=bienvenu

Comment 48

[nhottanscp]

Checked in to the trunk.

Comment 49

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

I have a problem with this fix.

here's the code as it stands now:

  tempName = GenerateFileNameFromURI(mURL); // Make it a sane name
#ifdef XP_WIN
  if (tempName && PL_strchr(tempName, '|'))
  {
    PR_Free(tempName);
    tempName = nsnull;
  }
#endif
#ifdef XP_MAC
  if (tempName && PL_strlen(tempName) >= 31)
    PR_DELETE(tempName)
#endif

basically, you are hacking it so if GenerateFileNameFromURI() doesn't generate 
a sane name, you throw it away and use "nsmail-X.tmp".

my issue with the code is it isn't XP (what about OS/2?)  about about other 
characters?

instead of tacking on changes using #ifdef XP_foo, you should fix 
GenerateFileNameFromURI() to really generate a "sane name"

I'd fix GenerateFileNameFromURI() to use NS_MsgHashIfNecessary(), which is how 
we've been turning a given string into something safe for the underlying OS.

it will take care of long file names and file names with illegal chars.

Comment 50

[nhottanscp]

For this bug, I intended to do a small change for this specific problem and 
tried not to affect others (because of nsBranch). So I did the same thing that 
is already working for Macintosh.
If there is no actual problem of creating "nsmail-X.tmp" then I would like to 
land this to the branch.
I did not know about NS_MsgHashIfNecessary(). Your suggested change seems to be 
the right one. By doing that, we could remove those ifdef code. Can that be done 
by a separate bug?

Comment 51

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

I've logged #89090 on the code cleanup.

Comment 52

[Frank Tang]

seth- do you agree it is low risk and right thing to do to land the current
patch into moz0.9.2 branch as vtrunk+ rule ?

Comment 53

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

ftang, yes I think the fix is low risk.

I'm not 100% convinced it is correct.  nhotta should double check that there 
aren't other illegal characters (or illegal lengths) that will cause the same 
problem.

Comment 54

[nhottanscp]

Xianglan tested cases which covers possible overwrap between Shift_JIS and ASCII.
I think other CJK file system charsets are hi bit on so do not have this problem.

> ------- Additional Comments From [e-mail] 2001-06-29 17:44 -------
> 
> I tried the filename containing characters in 0x8940 to 0x897E, only 897c 
> character causes this problem.
> 

Comment 55

[Frank Tang]

mark it as nsbranch+

Comment 56

[Frank Tang]

pdt+ per pdt meting. Land it today

Comment 57

[Frank Tang]

fix and land ito m92

Comment 58

[Frank Tang]

nhotta-
should we land this into trunk untill 89090 happen ?

Comment 59

[Frank Tang]

sorry, I am confused, this have already been check into trunk. Mark it fixed. 

Comment 60

[ji]

Verified with 07/11 branch build.

Comment 61

[ji]

Verified with 08/28 trunk build. It's fixed.