Bug 40452

Summary: Running 'fc-match --all' core dumps when no fonts are installed
Product: fontconfig Reporter: Arvind Umrao <arvind.umrao>
Component: fc-matchAssignee: Arvind Umrao <arvind.umrao>
Status: RESOLVED FIXED QA Contact: Behdad Esfahbod <freedesktop>
Severity: normal    
Priority: medium CC: akira, freedesktop
Version: 2.8   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description Arvind Umrao 2011-08-29 02:43:43 UTC
Running 'fc-match --all' core dumps when no fonts are installed. FcFontSort interface returns Null, when no fonts are installed. Put a check for Null condition at fc-match

Steps to reproduce with solaris :

1) Install system without fonts (server install) or boot to terminal in LiveCD install with sol-11-dev-167-text-x86.iso
2) Boot image in text mode
3) Select language Default 3, English
4) Select Shell at installation menu
5)fc-match --all 
It will core dump
Comment 1 Akira TAGOH 2011-09-01 19:52:49 UTC
*** Bug 40453 has been marked as a duplicate of this bug. ***
Comment 2 Akira TAGOH 2011-09-01 23:10:23 UTC
Proposed fix for this: http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz40452&id=08bfad59a670ec817910d17baab09ff5c69c66b2

fc-match -s and fc-match -a works without the segfault then.
Comment 3 Arvind Umrao 2011-09-01 23:36:36 UTC
It is better to give some sort of error messages.

proposed fixes 

diff --git a/fc-match/fc-match.c b/fc-match/fc-match.c
index e64b4bc..8b59896 100644
--- a/fc-match/fc-match.c
+++ b/fc-match/fc-match.c
@@ -175,6 +175,12 @@ main (int argc, char **argv)
 	int	j;
 	font_patterns = FcFontSort (0, pat, all ? FcFalse : FcTrue, 0, &result);
 
+	if (!font_patterns)
+	{
+	    fprintf (stderr, "Can't find any matching font\n");
+	    return 1;
+	}
+
 	for (j = 0; j < font_patterns->nfont; j++)
 	{
 	    FcPattern  *font_pattern;
-- 1.7.6
Comment 4 Behdad Esfahbod 2011-09-02 08:02:06 UTC
How about this:

diff --git a/src/fcmatch.c b/src/fcmatch.c
index 1b9162b..95ed898 100644
--- a/src/fcmatch.c
+++ b/src/fcmatch.c
@@ -686,7 +686,7 @@ FcFontSetSort (FcConfig         *config,
        nnodes += s->nfont;
     }
     if (!nnodes)
-       goto bail0;
+       return FcFontSetCreate ();
 
     for (nPatternLang = 0;
         FcPatternGet (p, FC_LANG, nPatternLang, &patternLang) == FcResultMatch;
Comment 5 Arvind Umrao 2011-09-02 10:00:51 UTC
(In reply to comment #4)
> How about this:
> 
> diff --git a/src/fcmatch.c b/src/fcmatch.c
> index 1b9162b..95ed898 100644
> --- a/src/fcmatch.c
> +++ b/src/fcmatch.c
> @@ -686,7 +686,7 @@ FcFontSetSort (FcConfig         *config,
>         nnodes += s->nfont;
>      }
>      if (!nnodes)
> -       goto bail0;
> +       return FcFontSetCreate ();
> 
>      for (nPatternLang = 0;
>          FcPatternGet (p, FC_LANG, nPatternLang, &patternLang) ==
> FcResultMatch;

If we handle null condition at fc-match/fc-match.c, we can give error message to user. At src/fcmatch.c, it would be difficult to give error message.  I can see at many other place in fc-match.c file null condition has been checked.
Comment 6 Behdad Esfahbod 2011-09-02 13:12:09 UTC
We sure should check for NULLs there, but FcFontSort() should not return NULL if no fonts are found.  FcFontMatch should I guess.
Comment 7 Arvind Umrao 2011-09-02 20:05:33 UTC
(In reply to comment #6)
> We sure should check for NULLs there, but FcFontSort() should not return NULL
> if no fonts are found.  FcFontMatch should I guess.

I think bail out was right, because no font installed in system.
Comment 8 Akira TAGOH 2011-09-05 18:17:43 UTC
(In reply to comment #6)
> We sure should check for NULLs there, but FcFontSort() should not return NULL
> if no fonts are found.  FcFontMatch should I guess.

Well, IMHO if FcFontSet *sets is NULL, FcFontSetSort should returns NULL as well. thus that makes sense (in this case at least) that FcFontSort() gives a pointer of FcFontSet that contains a reference of FcFontSet in FcConfig.
Comment 9 Behdad Esfahbod 2011-09-07 13:35:13 UTC
We need fixes in multiple places.
Comment 10 Akira TAGOH 2011-10-06 05:24:42 UTC
trivial updates after looking at other source code:
http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz40452&id=4c4733f292433affb02645cafeff1ef785ab7671

FWIW this change may breaks firefox perhaps. they are only relying on the null result for validation. so this may causes the unexpected result.

Though there seems no problem on Pango and Qt unless I'm missing anything else.
Comment 11 Arvind Umrao 2011-10-06 07:17:21 UTC
(In reply to comment #10)
> trivial updates after looking at other source code:
> http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz40452&id=4c4733f292433affb02645cafeff1ef785ab7671
> 
> FWIW this change may breaks firefox perhaps. they are only relying on the null
> result for validation. so this may causes the unexpected result.
> 
> Though there seems no problem on Pango and Qt unless I'm missing anything else.

You might want to assert result before if(result) condition check. 
like this way
 
@@ -672,6 +683,19 @@ FcFontSetSort (FcConfig *config,
FcBool *patternLangSat;
FcValue patternLang;

+ assert (sets != NULL);
+ assert (p != NULL);
+ assert (result != NULL);

+ /* There are some implementation that relying on the result of
+ * "result" to check if the return value of FcFontSetSort
+ * is valid or not.
+ * So we should initialize it to the conservative way since
+ * this function doesn't return NULL anymore.
+ */
+ if (result)
+ *result = FcResultNoMatch;
+
Comment 12 Akira TAGOH 2011-10-06 10:16:40 UTC
not a big deal. either way should works.
Comment 13 Akira TAGOH 2012-02-20 22:31:22 UTC
Fixed in a18ca17b.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.