XIM is disabled by bad font lookup

VERIFIED FIXED

Status

()

Core
Internationalization
VERIFIED FIXED
16 years ago
8 years ago

People

(Reporter: Masaki Katakai, Assigned: Masaki Katakai)

Tracking

({fixedOEM, inputmethod, intl})

Trunk
Sun
Solaris
fixedOEM, inputmethod, intl
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

16 years ago
I got a report that XIM does not work at all on Solaris 8 ru.ansi locale
and ru.koi8-r locales.

gdk_fontset_load("-*-*-medium-r-*-*-16-*-*-*-*-*-*-*");

does not match ansi fonts and koi8-r fonts at all in those
locales. Actually these locales are providing such fonts
but there is not -medium- fonts.

In earlier, we used "-*-*-*-*-*-*-16-*-*-*-*-*-*" for lookup
and changed to "medium-r-*-*-16" because italic/bold fonts
are picked up.

We should try more pattern as the following order
here if the first lookup is failed,

-*-*-medium-r-*-*-16-*-*-*-*-*-*-*
-*-*-*-r-*-*-16-*-*-*-*-*-*-*
-*-*-*-*-*-*-16-*-*-*-*-*-*-*
(Assignee)

Comment 1

16 years ago
Created attachment 88088 [details] [diff] [review]
patch
(Assignee)

Comment 2

16 years ago
Frank, could you r= please?
Status: NEW → ASSIGNED

Updated

16 years ago
Keywords: intl
(Assignee)

Comment 3

16 years ago
Created attachment 88235 [details] [diff] [review]
another patch using "*,*,*" form for gdk_fontset_load()
(Assignee)

Comment 4

16 years ago
Another way is to provide 

"-*-*-medium-r-*-*-%d-*-*-*-*-*-*-*,-*-*-*-r-*-*-%d-*-*-*-*
-*-*-*,-*-*-*-*-*-*-%d-*-*-*-*-*-*-*"

to gdk_fontset_locad() in patch 88088.

Which is better?

Blizzard, could you please give suggestion? and
r=/sr= please?


(Assignee)

Comment 5

16 years ago
Blizzard, could you take a look?
hmm, I don't actually have an opinion here.
(Assignee)

Updated

16 years ago
Attachment #88088 - Attachment is obsolete: true
(Assignee)

Comment 7

16 years ago
Thanks Blizzard,

attachment 88235 [details] [diff] [review] seems smarter.

Could you please r=/sr= please?

Updated

16 years ago
Attachment #88235 - Flags: review+

Comment 8

16 years ago
Comment on attachment 88235 [details] [diff] [review]
another patch using "*,*,*" form for gdk_fontset_load()

katakai:
> @@ -3583,7 +3585,7 @@
>      gdk_font_unref(gPreeditFontset);
>    }
>    char xlfdbase[128];
> -  sprintf(xlfdbase, "-*-*-medium-r-*-*-%d-*-*-*-*-*-*-*", height);
> +  sprintf(xlfdbase, XIC_FONTSET, height, height, height);

Nit: Could you add a comment here that the number of |height| parameters must
match the |XIC_FONTSET| string (and the ref above the |#define XIC_FONTSET ...|
to adjust the |sprintf()|-call on demand ?), please ?

r=Roland.Mainz@informatik.med.uni-giessen.de
(Assignee)

Comment 9

16 years ago
Created attachment 91330 [details] [diff] [review]
add comments
Attachment #88235 - Attachment is obsolete: true
(Assignee)

Comment 10

16 years ago
Thanks, Roland. I added the comments for number of arguments.

Updated

16 years ago
Attachment #91330 - Flags: review+

Comment 11

16 years ago
Can someone give sr= for the patch please?  ftang@netscape.com?
yokoyama@netscape.com?  cc yokoyama@netscape.com as well.  Thanks.

Comment 12

16 years ago
Margaret: none of us (ftang,yokoyama) have /sr privilage.  

kin: can you /sr?  I can't think of anyone else. (Solaris/XIM).......

Comment 13

16 years ago
Comment on attachment 91330 [details] [diff] [review]
add comments

Sorry for the delay, I was on vacation all last week.

The changes look ok to me ... but with my security hat on, the fact that we are
calling sprintf with a 128 byte buffer (xlfdbase), a formatting string
(XIC_FONTSET) which is already 94 characters long and expects 3 ints, makes me
wonder if the buffers need to be a bit larger?

On a 32 bit system an int looks like it can contain a number that is 11 chars
long (10 digits plus a sign). 3 of those numbers would put us up to the limit,
but what about 64 bit systems.

I'm not sure where we're getting the heights we're using in the sprintfs()
from, or if their even exploitable.

Just thought I'd ask. :-)
(Assignee)

Comment 14

16 years ago
Created attachment 95230 [details] [diff] [review]
use PR_smprintf
Attachment #91330 - Attachment is obsolete: true
(Assignee)

Comment 15

16 years ago
Hi Kin, thanks for comment.

Sure. now I use PR_smprintf().

Could you sr= please?
Comment on attachment 95230 [details] [diff] [review]
use PR_smprintf

sr=blizzard
Attachment #95230 - Flags: superreview+
(Assignee)

Comment 18

16 years ago
Thank you very much for review, blizzard and Roland.

Patch checked in to Trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: branchOEM

Comment 19

16 years ago
I am marking this bug as VERIFIED. 
Since we don't have a Sun/Solaris here to verify it, please reopen if anything 
wrong.
Status: RESOLVED → VERIFIED

Updated

16 years ago
Whiteboard: branchOEM → branchOEM+
(Assignee)

Comment 20

16 years ago
patch checked in to OEM branch.
Whiteboard: branchOEM+ → branchOEM+,fixedOEM

Updated

16 years ago
Whiteboard: branchOEM+,fixedOEM → fixedOEM

Updated

16 years ago
Keywords: fixedOEM
Whiteboard: fixedOEM
You need to log in before you can comment on or make changes to this bug.