Closed
Bug 225125
Opened 21 years ago
Closed 21 years ago
open / save dialog (file picker) causes crash with files with characters outside the repertoire of the current locale encoding in their names
Categories
(Core :: Internationalization, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mikael, Assigned: jshin1987)
References
Details
(Keywords: crash, intl)
Attachments
(1 file, 1 obsolete file)
2.61 KB,
patch
|
smontagu
:
review+
darin.moz
:
superreview+
mkaply
:
approval1.6+
|
Details | Diff | Splinter Review |
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•21 years ago
|
||
what does "locale" output on your system?
![]() |
Reporter | |
Comment 2•21 years ago
|
||
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•21 years ago
|
||
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).
Assignee: file-handling → smontagu
Status: UNCONFIRMED → NEW
Component: File Handling → Internationalization
Ever confirmed: true
QA Contact: amyy
![]() |
||
Comment 4•21 years ago
|
||
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... ;)
Assignee | ||
Comment 5•21 years ago
|
||
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•21 years ago
|
||
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....
Flags: blocking1.6?
![]() |
||
Comment 7•21 years ago
|
||
*** Bug 219969 has been marked as a duplicate of this bug. ***
Updated•21 years ago
|
Summary: open / save dialog causes crash with certain files in current/home directory → open / save dialog (file picker) causes crash with certain files (non-ascii characters) in current/home directory
Assignee | ||
Comment 8•21 years ago
|
||
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.
Assignee: smontagu → jshin
Keywords: intl
Summary: open / save dialog (file picker) causes crash with certain files (non-ascii characters) in current/home directory → open / save dialog (file picker) causes crash with files with characters outside the repertoire of the current locale encoding in their names
Assignee | ||
Comment 9•21 years ago
|
||
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)
Attachment #135131 -
Attachment is obsolete: true
Assignee | ||
Comment 10•21 years ago
|
||
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?
Attachment #137696 -
Flags: superreview?(darin)
Attachment #137696 -
Flags: review?(dougt)
Updated•21 years ago
|
Flags: blocking1.6? → blocking1.6+
![]() |
||
Updated•21 years ago
|
Attachment #137696 -
Flags: superreview?(darin) → superreview+
![]() |
||
Comment 11•21 years ago
|
||
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.
Attachment #137696 -
Flags: review?(dougt) → review+
Assignee | ||
Comment 12•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
If we want this for 1.6, someone who feels confident about the patch needs to
request approval...
Assignee | ||
Comment 16•21 years ago
|
||
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.
Attachment #137696 -
Flags: approval1.6?
Comment 17•21 years ago
|
||
Comment on attachment 137696 [details] [diff] [review]
a patch
a=mkaply - please get this in quickly.
Attachment #137696 -
Flags: approval1.6? → approval1.6+
Assignee | ||
Comment 18•21 years ago
|
||
fix checked into the trunk
Assignee | ||
Comment 19•21 years ago
|
||
sorry for bug spam. s/the trunk/the 1.6branch/________________________
for further work discussed, I'm keeping it open._
Status: NEW → ASSIGNED
Comment 20•21 years ago
|
||
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...
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 21•21 years ago
|
||
*** Bug 236927 has been marked as a duplicate of this bug. ***
Comment 22•21 years ago
|
||
*** Bug 217724 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•