133216 - multibyte decoders not robust enough on coming across an invalid octet

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Erich 'Ricky' Iseli]

I've installed AMI, a Korean Input server that converts keystrokes to Hangul.
Not only did it do that but also all File picker dialogs have now the dates in
Korean format (that's OK with me) but the worst thing: file names don't show...

I'm going to attach a screenshot.

Comment 1

[Erich 'Ricky' Iseli]

Attachment: Screenshot of filepicker and AMI icon

Comment 2

[Erich 'Ricky' Iseli]

Note that the filepicker is OK when opened the first time in the session (names
of files and folders appear). But as soon as you select another folder, the
dialog gets mangled.

If in the same session, you invoke another file picker, it's even worse: no
files, no directory. Nothing. Attaching another screenshot showing this.

Comment 3

[Erich 'Ricky' Iseli]

Attachment: Completely mangled filepicker

Comment 4

[Yuying Long]

cc Jungshik Shin

Comment 5

[Jungshik Shin]

It's not AMI but Korean locale under which Mozilla has to be launched
to work with AMI that changes date and time format of file picker.
Anywa, I almost always run Mozilla under Korean locale (ko_KR.EUC-KR)
and I've never seen this problem. I found that your screenshot
was taken with modern theme while I was running Mozilla with classic
theme. I changed my theme, but still file picker works fine with
0.9.9. What kind of files do you have in the directory?
Do they have Korean file names (in EUC-KR) or English or
other language file names? 
Could you try the following and upload a screenshot or two?

In born-shell like shells(e.g. bash)
$ cd {where mozilla binary is installed}
$ LC_ALL=C  ./run-mozilla.sh

In C-shell like shells (e.g. tcsh)
% cd {where mozilla binary is installed}
% env LC_ALL=C ./run-mozilla.sh
 
BTW, it'd also help if you specify the build id 
of Mozilla with this problem along with Linux
version/distribution and the window manager/desktop
you use. 

Comment 6

[Roy Yokoyama]

katakai-san: can you also take a look at this?

Comment 7

[Erich 'Ricky' Iseli]

Hi Jungshik Shin,

Ok, after tweaking a bit, I can give you some more details. I am running KDE2 on
Mandrake 8.1 and have the following content in my /etc/sysconfig/i18n file:

SYSFONT=lat0-sun16
LC_CTYPE=de_CH
LC_MONETARY=de_CH
LANGUAGE=de_CH:de
LC_TIME=de_CH
LC_NUMERIC=de_CH
LC_COLLATE=de_CH
LC_MESSAGES=de_CH
LANG=de
SYSFONTACM=iso15

I read instructions on a website that I should add to this file the following
lines, which I did (without really understanding - but it worked)

LC_ALL=ko
ENC=kr
XIM=Ami
XMODIFIERS="@im=Ami"

From now on, on every log in, Ami is started and the Hangul displays well in its
configuration dialog (I haven't figured out yet how to actually *type* hangul -
I don't know enough of Korean to understand that dialog - maybe you could give
me a hint privately?)

I did as you said also to start Mozilla from the shell, and it worked correctly.
If I run just ./mozilla, the dialog picker gets mangled. The filenames, by the
way, are all just plain English, so I really have no explanation why this happens.  

Comment 8

[Masaki Katakai]

Hi Erich,

so your problem went way? or still existing?

Comment 9

[Erich 'Ricky' Iseli]

Masaki Katakai, the problem is still existing, if I run Mozilla with just
"./mozilla"

However, if I do it with

$ cd {where mozilla binary is installed}
$ LC_ALL=C  ./run-mozilla.sh

Then everything is OK

My analysis is that Mozilla cannot handle this mixture of Swiss German and
Korean locale. I agree this is a very special situation...

Comment 10

[Erich 'Ricky' Iseli]

And while LC_ALL=C makes that my filepicker gets not mangled, I cannot use AMI
with Mozilla (like writing Mails in Korean doesn't work): the Ami-Icon usually
changes when the application lets me write in Korean. This works if I run
mozilla normally, but doesn't work any more when LC_ALL=C is set.

So this LC_ALL=C should be considered just a debug workaround to find out why
the filepicker gets mangled, but not a workaround to use Ami with Mozilla.

Also, I noticed that if I'm trying to open one of those "no-name" files
nevertheless, it doesn't work if the filepicker is mangled. Even worse, if I
change the file types to be displayed from .htm to *.*, Mozilla crashed.

Comment 11

[Masaki Katakai]

Hi Erich,

Please find AMI is working or not on your environment first.
Is AMI working on other gtk apps? e.g. gedit. Mozilla is
using gtk toolkit, so if gedit is working with AMI, Mozilla
should also work properly.

Also even when AMI is not working, Mozilla should start
in Korean locale. Yes, you can not use AMI on Mozilla at
the case, but we can find whether AMI really breaks Mozilla
file picker...

Please try,

1. start terminal
2. on the terminal export LANG=ko, LC_ALL=ko
   % export LC_ALL=ko
   % export LANG=ko
3. start AMI, please make sure XIM server also should run in
   ko locale
   % Ami &
4. start gedit and verify AMI is working on gedit
  % gedit

5. start Mozilla on the terminal
  If gedit works, Mozilla should work.
  Try to input Korean characters into URL field.

6. Open file picker...
  Works?

Please exit Mozilla and kill Ami input method server...

7. start Mozilla again
   Without Ami, Mozilla should start in ko locale

8. Open file picker
  Works?

Comment 12

[Jungshik Shin]

  Firstly,  do you have any problem with the file picker in a 
KDE application? How about gtk applications? For instance, 
in gedit, when you try File|Open, do you have the same problem
as in Mozilla? 

  If that's the case, please go to the last two paragraphs in this comment
(I'm very sorry to write this way .....) 

> And while LC_ALL=C makes that my filepicker gets not mangled, I cannot use AMI
> with Mozilla (like writing Mails in Korean doesn't work): the Ami-Icon usually
> changes when the application lets me write in Korean. 

  Yup, that's the case. 

> This works if I run mozilla normally,
 
  So, you figured out how to switch between Korean input mode
and English input mode in AMI, didn't you?  


> but doesn't work any more when LC_ALL=C is set.

 Mozilla can connect to AMI only when it's launched under a Korean locale
(either ko_KR.eucKR or ko_KR.UTF-8). When Mozilla is launched under C locale,
it cannot communicate with AMI and you cannot enter Korean with AMI.
You were able to enter Korean with AMI when you ran Mozilla without LC_ALL=C,
right?

> So this LC_ALL=C should be considered just a debug workaround to find out why
> the filepicker gets mangled, but not a workaround to use Ami with Mozilla.

  As I wrote in my first comment, I'm almost sure
that AMI is not the cause of your problem (file-picker
issue) If there's
a problem at all, it MUST be a Korean locale under which you launched Mozilla
to make it communicate with AMI. Accordingly, the summary line for
this bug should be 'Under Korean locale, filepicker gets mangled'. 

  Why don't you just restore your /etc/sysconfig/i18n to the original and restart
KDE so that the locale is not set to a Korean locale and AMI doesn't get launched 
automatically on KDE start-up?

   Then, you may try the following:

   $ LC_ALL=C ./run-mozilla.sh
   $ LC_ALL=ja ./run-mozilla.sh
   $ LC_ALL=ko ./run-mozilla.sh

In each of three cases, check out whether the filrepicker works fine.

Now, let's put in AMI just in case (although I don't think that's
the cause). 

  $ export XMODIFIERS=@im=Ami
  $ LC_ALL=ko ami &

Repeat three cases above.
  $ LC_ALL=ko mozilla   (or ./run-mozilla.sh) 
  $ LC_ALL=C  mozilla
  $ LC_ALL=ja mozilla 

If you find the filepicker works fine in all three cases both with and without
AMI running, we have to look somewhere
else. My suspicision goes to  'ENC=kr'.  Every Linux distribution comes up 
with its own way of setting up I18N environment (unfortunately,
none of them is as good as that of WIn2k/XP). I don't know what Mandrake 8.1 
is doing with 'ENC=kr' and with SYSFONT and SYSFONTACM(see below)  


Altarnatively,  SYSFONT, SYSFONTACM, LANGUAGE and
LANG   which are
left alone when ENC and LC_ALL are set to Korean in your /etc/sysconfig/i18n
can be the cause.  Especially, LANGUAGE and LANG are very likely
to be refered to by Mandrake when setting up gtk fontset and other gtk
configuration.
(again, it can sometime really tricky to figure out what each distribution
is doing with I18n-related environment variables )
Should that affect Mozilla? Like I wrote at the beg., if that affects
the filepicker in other gtk applications, it could affect Mozilla as well.
Still, I can't draw a consistent/coherent picture here.

Let me get back to you after you conduct experiments I suggested above.

Comment 13

[Erich 'Ricky' Iseli]

Masaki Katakai and Jungshik Shin, I'm gonna go through the testcases you
mentioned when I have a little more time than right now.

However, I think I found out the real problem: Even though I usually don't use
special characters in directory and file names, it happens I am running a German
Localization of Mandrake/KDE. On my desktop, guess how my Trash is called?
"Mülleimer". And this 'ü' sign ist the cause of all bad things... I cannot
rename this object and even if I could, I think this is a bug in Mozilla that
must be corrected.

'cause when I'm using other GTK applications (you mentioned GEdit, Gimp), other
than seing garbage displayed instead of the 'ü' (actually some hanja sign
instead of 'ül'), there's absolutely no problem in using the filepickers there.

My guess is that Mozilla is unable to understand the 'ül' combination, then
refuses to display the name from there on, and if I invoke the filepicker a
second time, it displays like in the second screenshot attached.

So changing the Summary of this bug:
old: AMI (Korean Input Server) breaks all file picker dialogs
new: Filepicker broken in Korean locale if filename in iso-8859-1

Feel free to correct it if you think I'm not precise enough. 

Comment 14

[Erich 'Ricky' Iseli]

Some more info here:
It definitely is the Korean locale.

First I restored the initial values of my i18n file

>   $ LC_ALL=C ./run-mozilla.sh
Filepicker works, Mülleimer displayed as M?lleimer
 
>   $ LC_ALL=ja ./run-mozilla.sh
Filepicker works, Mülleimer displayed as M?lleimer

>   $ LC_ALL=ko ./run-mozilla.sh
Filepicker broken, Mülleimer name NOT displayed (as in Attachment 1 [details] [diff])

Comment 15

[Erich 'Ricky' Iseli]

Sorry for the spam... The above testing was performed without AMI running.

Comment 16

[Jungshik Shin]

Thank you, Erich, for details. 
Now I can reproduce it. It's really strange that this happens
only with Korean locale. Even more strange is that 
the problem goes away if I apply either of two patches
I suggested for bug 131388.

u-umlaut is 0xFC in ISO-8859-1. When it's embedded among
US-ASCII chars (M, l, l, e, etc), EUC-KR decoder just flags
0xFC as an invalid char while EUC-JP and GB2312 decoder
flag 0xFC and the following octet 0x6C('l') as invalid.
Try Japanese(EUC), SC(GB2312) and Korean(EUC-KR) encoding at
http://jshin.net/moztest/mullerimer.html and count
the number of l's following what's supposed to be u-umlaut
which is rendered as '?'. With character coding set to EUC-JP
and GB2312, a single 'l' follows '?' whereas two l's follow
'?' with char. coding set to EUC-KR. 

This difference in decoder behavior should not present
any problem to the filepicker, but it does for yet-to-be
identified reason(s). 

I thought the gtk filepicker widget has a trouble
dealing with an isolated invalid char. However, this
can't be the case because after applying either
of two patches for bug 131388, the mozilla filepicker
lists 'M?llerimer' (i.e. only u-umlaut is converted
to '?' instead of 'u-umlaut' and the following 'l').

BTW, I can't find gtk-specific filepicker code
while there are separate cpp sources for Mac, OS/2, and BeOS.
Maybe, I have to try to set a breakpoint on xp filepicker
code and see what's going on...
 

Comment 17

[Jungshik Shin]

Attachment: a patch (not so pretty?!??)

This bug fall thru 'an unfilled hole' in 
nsUnicodeDecoderHelper.cpp and uscan.c for
94 x 94 2byte encodings (e.g. EUC-KR). I filled
the hole, but I'm still wondering if there can be 
a better way to fix this...

Comment 18

[Jungshik Shin]

Sorry for spamming. My patch(attachment 77208 [details] [diff] [review]) breaks the CP949 decoder 
and potentially other multitable decoders.

Comment 19

[Masaki Katakai]

Hi Jungshik,

Sorry I haven't looked at the detail yet,

but the patch of bug 131161 will help ?

Comment 20

[Jungshik Shin]

Attachment: a patch approaching the issue approaching from a diff. angle

Hello, Katakai-san,

Thank you for your note. 

The problem with EUC-KR decoder returning 'NS_OK_UDEC_MOREINPUT'
was identified earlier and I tried to solve in my previous
patch. 

I've since tried to figure out why not only filenames with chars. outside
the charset of the present locale(EUC-KR) but also 
any filenames after them became invisible. It turned out
that subsequent filenames are appended to the end
of the buffer used by the decoder as if they(filenames)
were coming from a stream. (somehow, the buffer is reset
when my sample html file mentioned in my earlier comment
is rendered by Mozilla with char. encoding set to EUC-KR)
A way to work around this
is to reset the buffer everytime FSToNewUCS is invoked
as is done in this patch.

Your patch to bug 131161, when applied without this patch,
would make all the subsequent files as well as the
file with an unconvertible name invisible. 

Therefore, I guess this patch has to be combined with
your patch to bug 131161 and applied together for
bug 131161. I'll add a comment to that effect in bug 131161.

As for this bug, I still think that I need to fill the
'hole' in nsUnicodeDecoderHelper.cpp and uscan.c I discovered.
With overlapping ranges in multitable converters, it's
a bit hard to do. (I believe this 'hole' was introduced
when support for decoders with overlapping ranges such
as Big5-HKSCS and CP949 was added.).

Comment 21

[Jungshik Shin]

Attachment: update of attachment 77208

I took back what I wrote about this patch( this
being not so pretty). I found
two precedents for 'scanner functions' returning 
PR_TRUE with what's guaranteed to fail in being
mapped in *out (they are  uCheckAndScanAlways4Byte()
and  uCheckAndScanAlways4ByteSwap()
for UTF-32 decoders) when they come across an invalid input
sequence.

> As for this bug, I still think that I need to fill the
> 'hole' in nsUnicodeDecoderHelper.cpp and uscan.c I discovered.

 A new patch filled the hole well. Currently,
uCheckAndScanAlways2ByteShiftGR() and
uCheckAndScanAlways2ByteGR128() returns PR_FALSE
when either inbuflen < 2 or the content of the inbuf
is invalid. Returning PR_FALSE results in NS_OK_UDEC_MOREINPUT
in  nsUnicodeDecodeHelper::ConvertByMultiTable(), which
is what it's supposed to do for inbuflen < 2 but
is not what we want for invalid input sequence.

With this patch,
uCheckAndScanAlways2ByteShiftGR() and
uCheckAndScanAlways2ByteGR128() returns PR_FALSE
*only* when the input buffer length is shorter than
required. For an invalid input content, it returns
PR_TRUE with 'out' set to 0xFF which is guaranteed
to fail in uMap(). This prevents 
nsUnicodeDecodeHelper::ConvertByMultiTable() from
returning NS_OK_UDEC_MOREINPUT. Instead,
an invalid input seq. is gracefully handled
by the code down the line.

A similar patch has to be applied to
several other 'scanner' functions (mainly
used by EUC-TW decoders) in uscan.c.
I also believe that uCheckAndScan4BytesGB18030()
has to be modified along the same line.

Comment 22

[Jungshik Shin]

Attachment: a more extensive patch to fix EUC-TW related functions as well

The differences between attachment 774333 [details] and this one are :

1) removed the check for the 1st byte of 94x94 chars. (94 x 128 )
   because the first byte is checked by a callee in nsUnicodeDecoderHelper.cpp
2) in addition to uCheckAndScanAlways2ByteShiftGR() and
uCheckAndScanAlways2ByteGR128(),  uCheckAndScan2ByteGRPrefix8F()
and uCheckAndScan2ByteGRPrefix8EAX (where x is [2-7]) 
are taken care of.

This will prevent decoders from keeping accumulating 
characters in the buffer after encountering an invalid
char. When coming across an invalid octet (in multibyte
encodings), the decoder (converter from multibyte encoding
to Unicode) currently returns NS_OK_UDEC_MOREINPUT.
This is not quite right because the more input from
a stream would not make valid what's already found to be
invalid. Instead, it should immediately raise the flag
and treat what's already found to be invalid as invalid.

When the patch for bug 64235 was checked in last spring,
this was not an issue. However, later last year, 
the logic in multitable decoder supporter routine changed
to support overlapped ranges. That change led this
fix to be necessary.  

BTW, I can't make attachment 77208 [details] [diff] [review] and 77433 obsolete. Shanjian,
could you do that for me? (bugzilla wouldn't let me change
the status of attachments even for bugs I'm assigned. this one
is not...)

Comment 23

[Masaki Katakai]

Jungshik, can you take this bug?

Comment 24

[Jungshik Shin]

Accepting this..

Comment 25

[Jungshik Shin]

Roy,Frank, Katakai-san, Shanjian,
Could any of you please review my patch?

Comment 26

[Masaki Katakai]

In bug 147333, we're now using iconv() for filename conversion.
I'm sorry I haven't tried yet but Is this bug still reproducible
with new codes and still needs to be fixed?

Comment 27

[Jungshik Shin]

Thank you for your prompt response.

> In bug 147333, we're now using iconv() for filename conversion.
> I'm sorry I haven't tried yet but Is this bug still reproducible
> with new codes and still needs to be fixed?

 Actually, my patch is not only for filepicker problem but also
tries to fix a more generic problem of encoding converters
not being robust enough. A fix for bug 1473333 may fix 
the original bug reported, but it wouldn't make  
'less robust than desirable' converters  more robust. 
See my comment 22 for details. 

I'm gonna change the summary line accordingly. 

Comment 28

[Erich 'Ricky' Iseli]

As reporter, I can just congratulate you for the great work. For me, the problem
is fixed and it's great to see that you are pushing the things even further.

Comment 29

[Roy Yokoyama]

Comment on attachment 78163 [details] [diff] [review]
a more extensive patch to fix EUC-TW related functions as well

/r=yokoyama; looks nice. Thanks, jshin

Comment 30

[Alec Flett]

Comment on attachment 78163 [details] [diff] [review]
a more extensive patch to fix EUC-TW related functions as well

the macro "CNS8EAX4BYTE" is pretty obtuse. Could we comeup with a better name,
or at least put some underscores to make this a little more readable? macro
names are cheap, we can make them nice and long :)

sr=alecf with a better name

Comment 31

[Jungshik Shin]

Attachment: same as before with underscores inserted for macroname per Alec's sr

Thank you for sr. I inserted two underscores and renamed it 
CNS_8EAX_4BYTE to enhance readability. Because it's used inside functions with
long and nice names as below :-), I think the macroname
needs not be very descriptive. 

 PRIVATE PRBool uCheckAndScan2ByteGRPrefix8EA2(
					      uShiftTable    *shift,
					      PRInt32*	  state,
					      unsigned char  *in,
					      PRUint16	  *out,
					      PRUint32	   inbuflen,
					      PRUint32*    inscanlen
					      )
{
  CNS_8EAX_4BYTE(0xA2)
}

Comment 32

[Alec Flett]

Comment on attachment 91131 [details] [diff] [review]
same as before  with underscores inserted for macroname per Alec's sr

thanks.. much more understandable. sr=alecf

Comment 33

[Asa Dotzler [:asa]]

Comment on attachment 91131 [details] [diff] [review]
same as before  with underscores inserted for macroname per Alec's sr

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.

Comment 34

[Jungshik Shin]

checked in to the trunk.
thank you.

Comment 35

[Yuying Long]

Change QA contact to Roy for this decoder code change issue, also I don't have a
Mandrake 8.1 to verify it.