225125 - open / save dialog (file picker) causes crash with files with characters outside the repertoire of the current locale encoding in their names

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Mikael Simonsson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6a) Gecko/20031029
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6a) Gecko/20031029

With filenames of certain length and with certain characters in the directory
mozilla tries to show in the open/save dialogs causes imidiate crash (you never
even see which directory it is trying to show)

Example of a filename: "ehy dajf oisa (2000) ks zz dd Z - jsadoif å 244728.txt"
(without the quotes and notice the character "å" dec: 134, hex: 86)

Reproducible: Always

Steps to Reproduce:
1. Command line: echo "test" > "ehy dajf oisa (2000) ks zz dd Z - jsadoif å
244728.txt"
2. Mozilla: Open-File and go to directory contaning the file created above.
3. Mozilla: Open dialog goes white, crash after 1-2 seconds.

Actual Results:  
Crash

Expected Results:  
Shown the files in the directory.

Same results on Mozilla 1.5.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

what does "locale" output on your system?

Comment 2

[Mikael Simonsson]

It was set to "POSIX", when I changed it to "sv_SE" Mozilla doesn't crash.
Thanks. But should POSIX really cause Mozilla to crash?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Given an nsCOMPtr<nsIFile> pointing to that file, if I call GetLeafName the
nsCOMPtr value gets clobbered to 0x1 (and the release() it does in its
destructor crashes, of course).  All this only in the POSIX locale so far...

If I replace NS_CopyNativeToUnicode with CopyASCIItoUCS2 in the GET_UCS macro,
things seem to be good...

Now the string we pass to the macro is in fact the leafname of that file (with
that non-ASCII char in it).

The problem, of course, is that NS_CopyNativeToUnicode is just busted.  It calls
nsNativeCharsetConverter::NativeToUnicode, which calls xp_iconv to do the
conversion.  If this fails, instead of returning an error (which would be
useful) it warns and tries to call isolatin1_to_utf16.

But the problem, is that it passes the same pointer-to-buffer to
isolatin1_to_utf16 that it passed to xp_iconv.  xp_iconv increments this
pointer-to-buffer (see "man 3 iconv", information on the outbuf**).  Oh, it
increments the input buffer pointer too.  So the point is, after the xp_iconv
call, the "input" and "output" pointers are bogus, especially if the conversion
failed... In any case, using the updated "input" but NOT the updated "inputLeft"
(which is what we do) is just wrong -- we will read off the end of the input
string, in addition to everything else.  So I suspect we simply end up running
off the end of the internal buffer of the output string and writing all over the
stack.  The fact that the filename is 71 chars means it's long enough for that
to happen (the internal buffer is only 80 chars, as I recall).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Illustration

This patch fixes the crash by keeping us from running off the end of the output
string... not that this makes us have anything like correct behavior, so it
shouldn't be checked in, but it does confirm my theory... ;)

Comment 5

[Jungshik Shin]

I'm wondering what to do with this bug. We might want to 'fix' this in steps. 
To prevent the crash, I guess bz's patch is fine except that the
equivalent has to be done in #UTF8_FALLBACK block. 

Then, we should do the right thing. That is, we have to handle errors from
iconv(3) more gracefully. Replacing unconvertible chars by '?' is one of
ways we can pursue.  

Comment 6

[Boris Zbarsky [:bzbarsky]]

The right way to fix this bug, imo, is to pass _copies_ of the pointers to the
data to iconv, so when it advances the pointers we still have the originals and
can try calling iconv with them in the fallback.

This bug totally slipped off my radar, but imo it should be a blocker for 1.6;
we need to either fix the crash or remove the fallback until we can do it right
for this release....

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Jungshik Shin]

re comment #6: yes, let's do the former. Later, we can work on what I suggested 
in the 2nd para of comment #5. Do you want me to make a patch (you're still out 
of town, right?)? As smontagu is on vacation, I'm taking it over.

Comment 9

[Jungshik Shin]

Attachment: a patch

After writing my last comment, I realized that the approach of attachment
135131 [details] [diff] [review](bz's original patch) is better than what he suggested later. With that
approach, we have a higher chance of showing filenames 'correctly' (at least up
to the point where an invalid input is detected) than otherwise. This patch is
an extension to attachment 135131 [details] [diff] [review] for platforms where UTF-8 fallback is used
(e.g. Solaris)

Comment 10

[Jungshik Shin]

Comment on attachment 137696 [details] [diff] [review]
a patch

>Index: xpcom/io/nsNativeCharsetUtils.cpp

>+    // back up input and ouput pointers for the fallback case
>+    const char *in = *input;
>+    PRUnichar *out = *output;
>+

Sorry, the above lines should have been removed before I made a patch.

With that in mind, would you review?

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 137696 [details] [diff] [review]
a patch

I'm not sure that I agree with comment 9, specifically that "With that
approach, we have a higher chance of showing filenames 'correctly' (at least up
to the point where an invalid input is detected) than otherwise."

If a filename contains an invalid codepoint in what we believed to be the
encoding, doesn't that make it more likely that what we already decoded
contained codepoints that were nominally valid, but still incorrect? I guess it
depends on the scenario, and which exact encodings are involved.

Since a crash is involved, the approach suggested in comment 5 makes sense.
r=smontagu and let's work on doing the right thing later.

Comment 12

[Jungshik Shin]

Yeah, now I agree with you (comment #11). After uploading the patch, asking for
r/sr, working on bug 229032 (where I want to replace some calls to our
converters with native charset utils),  I realized that either just returning an
error or implementing bz's second option (doing the fallback conversion from the
beginning instead of the place where invalid octets are found) is more 'robust'
even as the _first_ step. I'm landing it now because it'll stop crash and gonna
ask for a1.6


During 1.7-cycle, we have to decide exactly what to do (return NS_FAILURE_XXX,
NS_SUCCESS_YYYY, what fallback to use...). So, I'll keep this open.

Comment 13

[chris hofmann]

so did this make it into 1.6?  if so lets mark fixed, and reopen another bug for
the continuing work...  that will make sure the right things happen for testing,
verification, and tracking of 1.6 changes...

Comment 14

[Robert Parenton]

In response to Comment #13, according to the Tinderbox/Bonsai, this hasn't been
landed on the 1.6 branch:

http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?treeid=default&branch=MOZILLA_1_6_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=week&mindate=&maxdate=&cvsroot=%2Fcvsroot

Comment 15

[Boris Zbarsky [:bzbarsky]]

If we want this for 1.6, someone who feels confident about the patch needs to
request approval...

Comment 16

[Jungshik Shin]

Comment on attachment 137696 [details] [diff] [review]
a patch

asking for a1.6
(I wanted to try one more time before asking for a1.6)

This is to prevent Mozilla from crashing when it comes across a long filename
with characters not covered by the current locale encoding. 

affected platform : Linux and other Unix platforms with iconv


risk : low. this patch adds a fallback to prevent crash.

Comment 17

[Mike Kaply [:mkaply]]

Comment on attachment 137696 [details] [diff] [review]
a patch

a=mkaply - please get this in quickly.

Comment 18

[Jungshik Shin]

fix checked into the trunk

Comment 19

[Jungshik Shin]

 sorry for bug spam. s/the trunk/the 1.6branch/________________________
   for further work discussed, I'm keeping it open._

Comment 20

[chris hofmann]

need to mark this one fixed, and open a new bug for the future work.   that
ensures that testing, release note, and chance tracking logs for the release
will pick up this work as part of 1.6.   thx...

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 22

[Michael Lefevre]

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