* bug#12823: Invalid font name
@ 2012-11-07 15:15 Stefan Monnier
2012-11-07 16:52 ` Jan Djärv
2012-11-09 14:07 ` Kenichi Handa
0 siblings, 2 replies; 12+ messages in thread
From: Stefan Monnier @ 2012-11-07 15:15 UTC (permalink / raw)
To: 12823
Package: Emacs
Version: 24.3.50
After letting xft use bitmap fonts on my system, Emacs started to crash,
which was tracked to an invalid font name that was used
without checking.
So I added
if (NILP (spec))
signal_error ("Invalid font name", ascii_font);
to x_set_font in revno 110704, but this only prevents the crash,
replacing it with an error. Basically the error is that ascii_font is
not a valid XLFD font name because one of its fields has a "-" in its
name, and since fields are separated by "-", this leads to a misparse.
Now, this invalid name was built by Emacs, probably in
font_unparse_xlfd. The appended patch fixes my problem.
So now the question is: should we reorder all the entries in the
width_table, slant_table, and weight_table so that the first entry of
every line is a non-dashed name? Or could this have undesirable effects?
Stefan
=== modified file 'src/font.c'
--- src/font.c 2012-11-06 03:17:56 +0000
+++ src/font.c 2012-11-07 15:04:24 +0000
@@ -102,7 +102,7 @@
{ 50, { "ultra-condensed", "ultracondensed" }},
{ 63, { "extra-condensed", "extracondensed" }},
{ 75, { "condensed", "compressed", "narrow" }},
- { 87, { "semi-condensed", "semicondensed", "demicondensed" }},
+ { 87, { "semicondensed", "semi-condensed", "demicondensed" }},
{ 100, { "normal", "medium", "regular", "unspecified" }},
{ 113, { "semi-expanded", "semiexpanded", "demiexpanded" }},
{ 125, { "expanded" }},
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#12823: Invalid font name
2012-11-07 15:15 bug#12823: Invalid font name Stefan Monnier
@ 2012-11-07 16:52 ` Jan Djärv
2012-11-09 14:07 ` Kenichi Handa
1 sibling, 0 replies; 12+ messages in thread
From: Jan Djärv @ 2012-11-07 16:52 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 12823
Hello.
From the XLFD-specification 1.5 (http://www.xfree86.org/current/xlfd.pdf):
"Field values are constructed as strings of ISO 8859-1 graphic characters, excluding the following:
• ‘‘−’’ (HYPHEN), the XLFD font name delimiter character
• ‘‘?’’ (QUESTION MARK) and ‘‘*’’ (ASTERISK), the X protocol font name wildcard characters
• ‘‘,’’ (COMMA), used by Xlib to separate XLFD font names in a font set.
• ‘‘"’’ (QUOTATION MARK), used by some commercial products to quote a font name.
Alphabetic case distinctions are allowed but are for human readability concerns only. Conforming X servers will perform matching on font name query or open requests independent of case. The entire font name string must have no more than 255 characters."
So the use of semi-condensed (for example) is invalid, so we should simply remove all entries with "-" in them.
Jan D.
7 nov 2012 kl. 16:15 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
> Package: Emacs
> Version: 24.3.50
>
> After letting xft use bitmap fonts on my system, Emacs started to crash,
> which was tracked to an invalid font name that was used
> without checking.
> So I added
>
> if (NILP (spec))
> signal_error ("Invalid font name", ascii_font);
>
> to x_set_font in revno 110704, but this only prevents the crash,
> replacing it with an error. Basically the error is that ascii_font is
> not a valid XLFD font name because one of its fields has a "-" in its
> name, and since fields are separated by "-", this leads to a misparse.
>
> Now, this invalid name was built by Emacs, probably in
> font_unparse_xlfd. The appended patch fixes my problem.
>
> So now the question is: should we reorder all the entries in the
> width_table, slant_table, and weight_table so that the first entry of
> every line is a non-dashed name? Or could this have undesirable effects?
>
>
> Stefan
>
>
> === modified file 'src/font.c'
> --- src/font.c 2012-11-06 03:17:56 +0000
> +++ src/font.c 2012-11-07 15:04:24 +0000
> @@ -102,7 +102,7 @@
> { 50, { "ultra-condensed", "ultracondensed" }},
> { 63, { "extra-condensed", "extracondensed" }},
> { 75, { "condensed", "compressed", "narrow" }},
> - { 87, { "semi-condensed", "semicondensed", "demicondensed" }},
> + { 87, { "semicondensed", "semi-condensed", "demicondensed" }},
> { 100, { "normal", "medium", "regular", "unspecified" }},
> { 113, { "semi-expanded", "semiexpanded", "demiexpanded" }},
> { 125, { "expanded" }},
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#12823: Invalid font name
2012-11-07 15:15 bug#12823: Invalid font name Stefan Monnier
2012-11-07 16:52 ` Jan Djärv
@ 2012-11-09 14:07 ` Kenichi Handa
2012-11-13 11:55 ` Kenichi Handa
1 sibling, 1 reply; 12+ messages in thread
From: Kenichi Handa @ 2012-11-09 14:07 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 12823
In article <jwvpq3p78ch.fsf@iro.umontreal.ca>, Stefan Monnier <monnier@iro.umontreal.ca> writes:
> Now, this invalid name was built by Emacs, probably in
> font_unparse_xlfd. The appended patch fixes my problem.
> So now the question is: should we reorder all the entries in the
> width_table, slant_table, and weight_table so that the first entry of
> every line is a non-dashed name? Or could this have undesirable effects?
Yes. The first elements in those table should match the
face attribute values for :weight, :width, and :slant (see
set-face-attribute). I should have wrote that in the
comment.
Could you please try this patch instead?
---
Kenichi Handa
handa@gnu.org
=== modified file 'src/font.c'
--- src/font.c 2012-11-03 05:11:34 +0000
+++ src/font.c 2012-11-09 14:05:15 +0000
@@ -1234,8 +1234,19 @@
f[j] = "*";
else
{
+ int c, k, l;
+
val = SYMBOL_NAME (val);
- f[j] = SSDATA (val);
+ len = SBYTES (val);
+ f[j] = alloca (len + 1);
+ /* Copy the name while excluding '-', '?', ',', and '"'. */
+ for (k = l = 0; k < len; k++)
+ {
+ c = SREF (val, k);
+ if (c != '-' && c != '?' && c != ',' && c != '"')
+ f[l++] = c;
+ }
+ f[l] = '\0';
}
}
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#12823: Invalid font name
2012-11-09 14:07 ` Kenichi Handa
@ 2012-11-13 11:55 ` Kenichi Handa
2012-11-13 19:25 ` Andreas Schwab
0 siblings, 1 reply; 12+ messages in thread
From: Kenichi Handa @ 2012-11-13 11:55 UTC (permalink / raw)
To: Kenichi Handa; +Cc: 12823
In article <87ip9euaxb.fsf@gnu.org>, Kenichi Handa <handa@gnu.org> writes:
> Yes. The first elements in those table should match the
> face attribute values for :weight, :width, and :slant (see
> set-face-attribute). I should have wrote that in the
> comment.
> Could you please try this patch instead?
Oops, sorry, I've sent the wrong version. Here's the
correct patch.
=== modified file 'src/font.c'
--- src/font.c 2012-11-03 05:11:34 +0000
+++ src/font.c 2012-11-13 11:50:50 +0000
@@ -1185,7 +1185,7 @@
font_unparse_xlfd (Lisp_Object font, int pixel_size, char *name, int nbytes)
{
char *p;
- const char *f[XLFD_REGISTRY_INDEX + 1];
+ char *f[XLFD_REGISTRY_INDEX + 1];
Lisp_Object val;
int i, j, len;
@@ -1234,8 +1234,21 @@
f[j] = "*";
else
{
+ int c, k, l;
+ ptrdiff_t alloc;
+
val = SYMBOL_NAME (val);
- f[j] = SSDATA (val);
+ alloc = SBYTES (val) + 1;
+ if (nbytes <= alloc)
+ return -1;
+ f[j] = alloca (alloc);
+ /* Copy the name while excluding '-', '?', ',', and '"'. */
+ for (k = l = 0; k < alloc; k++)
+ {
+ c = SREF (val, k);
+ if (c != '-' && c != '?' && c != ',' && c != '"')
+ f[j][l++] = c;
+ }
}
}
---
Kenichi Handa
handa@gnu.org
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#12823: Invalid font name
2012-11-13 11:55 ` Kenichi Handa
@ 2012-11-13 19:25 ` Andreas Schwab
2012-11-15 13:30 ` Kenichi Handa
0 siblings, 1 reply; 12+ messages in thread
From: Andreas Schwab @ 2012-11-13 19:25 UTC (permalink / raw)
To: Kenichi Handa; +Cc: 12823
Kenichi Handa <handa@gnu.org> writes:
> === modified file 'src/font.c'
> --- src/font.c 2012-11-03 05:11:34 +0000
> +++ src/font.c 2012-11-13 11:50:50 +0000
> @@ -1185,7 +1185,7 @@
> font_unparse_xlfd (Lisp_Object font, int pixel_size, char *name, int nbytes)
> {
> char *p;
> - const char *f[XLFD_REGISTRY_INDEX + 1];
> + char *f[XLFD_REGISTRY_INDEX + 1];
This will provoke warnings that are turned into errors with
--enable-gcc-warnings. There is no need for that, just use a temporary
variable (there is already one above that is perfectly suitable).
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#12823: Invalid font name
2012-11-13 19:25 ` Andreas Schwab
@ 2012-11-15 13:30 ` Kenichi Handa
2012-11-27 14:26 ` Kenichi Handa
0 siblings, 1 reply; 12+ messages in thread
From: Kenichi Handa @ 2012-11-15 13:30 UTC (permalink / raw)
To: Andreas Schwab; +Cc: 12823
In article <m2390db90y.fsf@igel.home>, Andreas Schwab <schwab@linux-m68k.org> writes:
> Kenichi Handa <handa@gnu.org> writes:
> > === modified file 'src/font.c'
> > --- src/font.c 2012-11-03 05:11:34 +0000
> > +++ src/font.c 2012-11-13 11:50:50 +0000
> > @@ -1185,7 +1185,7 @@
> > font_unparse_xlfd (Lisp_Object font, int pixel_size, char *name, int nbytes)
> > {
> > char *p;
> > - const char *f[XLFD_REGISTRY_INDEX + 1];
> > + char *f[XLFD_REGISTRY_INDEX + 1];
> This will provoke warnings that are turned into errors with
> --enable-gcc-warnings. There is no need for that, just use a temporary
> variable (there is already one above that is perfectly suitable).
Thank you for the suggesiton. Do you mean something like
this additional patch?
=== modified file 'src/font.c'
--- src/font.c 2012-11-13 14:24:26 +0000
+++ src/font.c 2012-11-15 13:04:45 +0000
@@ -1185,7 +1185,7 @@
font_unparse_xlfd (Lisp_Object font, int pixel_size, char *name, int nbytes)
{
char *p;
- char *f[XLFD_REGISTRY_INDEX + 1];
+ const char *f[XLFD_REGISTRY_INDEX + 1];
Lisp_Object val;
int i, j, len;
@@ -1241,13 +1241,13 @@
alloc = SBYTES (val) + 1;
if (nbytes <= alloc)
return -1;
- f[j] = alloca (alloc);
+ f[j] = p = alloca (alloc);
/* Copy the name while excluding '-', '?', ',', and '"'. */
for (k = l = 0; k < alloc; k++)
{
c = SREF (val, k);
if (c != '-' && c != '?' && c != ',' && c != '"')
- f[j][l++] = c;
+ p[l++] = c;
}
}
}
---
Kenichi Handa
handa@gnu.org
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#12823: Invalid font name
2012-11-15 13:30 ` Kenichi Handa
@ 2012-11-27 14:26 ` Kenichi Handa
2012-11-27 15:10 ` Stefan Monnier
0 siblings, 1 reply; 12+ messages in thread
From: Kenichi Handa @ 2012-11-27 14:26 UTC (permalink / raw)
To: Kenichi Handa; +Cc: 12823
For fixing this bug, I've just installed this change to
trunk (rev. 111019). But, as the bug had caused crashing,
it may be better to apply the change to emacs-24.
---
Kenichi Handa
handa@gnu.org
=== modified file 'src/ChangeLog'
--- src/ChangeLog 2012-11-27 05:38:42 +0000
+++ src/ChangeLog 2012-11-27 13:40:38 +0000
@@ -1,3 +1,13 @@
+2012-11-18 Kenichi Handa <handa@gnu.org>
+
+ * font.c (font_unparse_xlfd): Fix previous change. Keep "const"
+ for the variable "f".
+
+2012-11-13 Kenichi Handa <handa@gnu.org>
+
+ * font.c (font_unparse_xlfd): Exclude special characters from the
+ generating XLFD name.
+
2012-11-27 Paul Eggert <eggert@cs.ucla.edu>
Assume POSIX 1003.1-1988 or later for grp.h, pwd.h.
=== modified file 'src/font.c'
--- src/font.c 2012-11-06 13:26:20 +0000
+++ src/font.c 2012-11-27 13:40:38 +0000
@@ -1234,8 +1234,21 @@
f[j] = "*";
else
{
+ int c, k, l;
+ ptrdiff_t alloc;
+
val = SYMBOL_NAME (val);
- f[j] = SSDATA (val);
+ alloc = SBYTES (val) + 1;
+ if (nbytes <= alloc)
+ return -1;
+ f[j] = p = alloca (alloc);
+ /* Copy the name while excluding '-', '?', ',', and '"'. */
+ for (k = l = 0; k < alloc; k++)
+ {
+ c = SREF (val, k);
+ if (c != '-' && c != '?' && c != ',' && c != '"')
+ p[l++] = c;
+ }
}
}
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#12823: Invalid font name
2012-11-27 14:26 ` Kenichi Handa
@ 2012-11-27 15:10 ` Stefan Monnier
2012-11-28 9:20 ` Kenichi Handa
0 siblings, 1 reply; 12+ messages in thread
From: Stefan Monnier @ 2012-11-27 15:10 UTC (permalink / raw)
To: Kenichi Handa; +Cc: 12823
> For fixing this bug, I've just installed this change to
> trunk (rev. 111019).
Thanks, that looks good.
> But, as the bug had caused crashing,
> it may be better to apply the change to emacs-24.
I didn't see any crashes, only assertion violations.
FWIW Debian's emacs23 and emacs24 binaries don't crash here, so
I suspect that the bug is luckily "solved" by accident somewhere.
I'm still wondering about those names that include -, such as
"semi-condensed" that we have in font.c:
- IIUC, they can never be useful for XFLD font names. Are they useful
for other font name formats supported by Emacs? Examples?
- If they are useful sometimes, is it important to have the dashed
name first in the list?
Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#12823: Invalid font name
2012-11-27 15:10 ` Stefan Monnier
@ 2012-11-28 9:20 ` Kenichi Handa
2012-11-28 14:57 ` Stefan Monnier
0 siblings, 1 reply; 12+ messages in thread
From: Kenichi Handa @ 2012-11-28 9:20 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 12823
In article <jwvip8r6qe1.fsf-monnier+emacs@gnu.org>, Stefan Monnier <monnier@iro.umontreal.ca> writes:
> > For fixing this bug, I've just installed this change to
> > trunk (rev. 111019).
> Thanks, that looks good.
> > But, as the bug had caused crashing,
> > it may be better to apply the change to emacs-24.
> I didn't see any crashes, only assertion violations.
> FWIW Debian's emacs23 and emacs24 binaries don't crash here, so
> I suspect that the bug is luckily "solved" by accident somewhere.
You wrote;
> After letting xft use bitmap fonts on my system, Emacs started to crash,
> which was tracked to an invalid font name that was used
> without checking.
> So I added
> if (NILP (spec))
> signal_error ("Invalid font name", ascii_font);
> to x_set_font in revno 110704, but this only prevents the crash,
> replacing it with an error.
Do you mean this workaround as "assertion violations"? I
think it just hides the deeper bug. Here, SPEC is a return
value of font_spec_from_name (fontset_ascii (fontset)), and
font_spec_from_name should never fail with that kind of
argument.
> I'm still wondering about those names that include -, such as
> "semi-condensed" that we have in font.c:
> - IIUC, they can never be useful for XFLD font names. Are they useful
> for other font name formats supported by Emacs? Examples?
> - If they are useful sometimes, is it important to have the dashed
> name first in the list?
As I wrote before, the table is also used to get a face
attribute value from a font spec, and, for instance, face
attribute :width allows the symbol `semi-condensed'. If we
don't have that name first in the list, we must have another
index specifying which name is a valid face attribute value,
or must have another tables that map numeric values of font
spec to face attribute values.
---
Kenichi Handa
handa@gnu.org
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#12823: Invalid font name
2012-11-28 9:20 ` Kenichi Handa
@ 2012-11-28 14:57 ` Stefan Monnier
2012-12-03 9:21 ` Kenichi Handa
0 siblings, 1 reply; 12+ messages in thread
From: Stefan Monnier @ 2012-11-28 14:57 UTC (permalink / raw)
To: Kenichi Handa; +Cc: 12823
>> After letting xft use bitmap fonts on my system, Emacs started to crash,
>> which was tracked to an invalid font name that was used
>> without checking.
>> So I added
>> if (NILP (spec))
>> signal_error ("Invalid font name", ascii_font);
>> to x_set_font in revno 110704, but this only prevents the crash,
>> replacing it with an error.
> Do you mean this workaround as "assertion violations"?
No, I mean that without this NILP check, subsequent code "crashed" with an
assertion violation (due to assuming that `spec' is a string, IIRC).
> I think it just hides the deeper bug. Here, SPEC is a return value of
> font_spec_from_name (fontset_ascii (fontset)), and font_spec_from_name
> should never fail with that kind of argument.
Right.
>> I'm still wondering about those names that include -, such as
>> "semi-condensed" that we have in font.c:
>> - IIUC, they can never be useful for XFLD font names. Are they useful
>> for other font name formats supported by Emacs? Examples?
>> - If they are useful sometimes, is it important to have the dashed
>> name first in the list?
> As I wrote before, the table is also used to get a face
> attribute value from a font spec, and, for instance, face
> attribute :width allows the symbol `semi-condensed'.
Sorry, I must have missed that message. So we can't just remove those
dashed names. Fine.
> If we don't have that name first in the list, we must have another
> index specifying which name is a valid face attribute value, or must
> have another tables that map numeric values of font spec to face
> attribute values.
My understanding is that this issue is only related to whether or not
`semi-condensed' is in the list, but is unrelated to whether it comes
before or after `semicondensed' in the list.
Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#12823: Invalid font name
2012-11-28 14:57 ` Stefan Monnier
@ 2012-12-03 9:21 ` Kenichi Handa
2012-12-03 17:51 ` Stefan Monnier
0 siblings, 1 reply; 12+ messages in thread
From: Kenichi Handa @ 2012-12-03 9:21 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 12823
In article <jwv4nk9235d.fsf-monnier+emacs@gnu.org>, Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
> > If we don't have that name first in the list, we must have another
> > index specifying which name is a valid face attribute value, or must
> > have another tables that map numeric values of font spec to face
> > attribute values.
> My understanding is that this issue is only related to whether or not
> `semi-condensed' is in the list, but is unrelated to whether it comes
> before or after `semicondensed' in the list.
And, what is your opinion on "whether or not
`semi-condensed' is in the list"?
---
Kenichi Handa
handa@gnu.org
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#12823: Invalid font name
2012-12-03 9:21 ` Kenichi Handa
@ 2012-12-03 17:51 ` Stefan Monnier
0 siblings, 0 replies; 12+ messages in thread
From: Stefan Monnier @ 2012-12-03 17:51 UTC (permalink / raw)
To: Kenichi Handa; +Cc: 12823
>> > If we don't have that name first in the list, we must have another
>> > index specifying which name is a valid face attribute value, or must
>> > have another tables that map numeric values of font spec to face
>> > attribute values.
>> My understanding is that this issue is only related to whether or not
>> `semi-condensed' is in the list, but is unrelated to whether it comes
>> before or after `semicondensed' in the list.
> And, what is your opinion on "whether or not
> `semi-condensed' is in the list"?
IIUC you said that `semi-condensed' should be in the list, and that's
fine by me. The remaining question is whether it should come first.
Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-12-03 17:51 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-07 15:15 bug#12823: Invalid font name Stefan Monnier
2012-11-07 16:52 ` Jan Djärv
2012-11-09 14:07 ` Kenichi Handa
2012-11-13 11:55 ` Kenichi Handa
2012-11-13 19:25 ` Andreas Schwab
2012-11-15 13:30 ` Kenichi Handa
2012-11-27 14:26 ` Kenichi Handa
2012-11-27 15:10 ` Stefan Monnier
2012-11-28 9:20 ` Kenichi Handa
2012-11-28 14:57 ` Stefan Monnier
2012-12-03 9:21 ` Kenichi Handa
2012-12-03 17:51 ` Stefan Monnier
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).