bug#12823: Invalid font name

bug#12823: Invalid font name

[Stefan Monnier]

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" }},

bug#12823: Invalid font name

[Jan Djärv]

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" }},
> 
> 
> 

bug#12823: Invalid font name

[Kenichi Handa]

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';
 	}
     }
 

bug#12823: Invalid font name

[Kenichi Handa]

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

bug#12823: Invalid font name

[Andreas Schwab]

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."

bug#12823: Invalid font name

[Kenichi Handa]

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

bug#12823: Invalid font name

[Kenichi Handa]

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;
+	    }
 	}
     }
 

bug#12823: Invalid font name

[Stefan Monnier]

> 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

bug#12823: Invalid font name

[Kenichi Handa]

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

bug#12823: Invalid font name

[Stefan Monnier]

>> 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

bug#12823: Invalid font name

[Kenichi Handa]

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

bug#12823: Invalid font name

[Stefan Monnier]

>> > 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