After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 748553 - GIMP crash if a font does not have read permission
GIMP crash if a font does not have read permission
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: General
2.8.10
Other Linux
: Normal major
: 2.10
Assigned To: GIMP Bugs
GIMP Bugs
: 758492 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-04-27 19:35 UTC by Micky
Modified: 2018-04-29 15:40 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Micky 2015-04-27 19:35:00 UTC
I had installed some fonts incorrectly (*)
I tried to use those fonts in a GIMP file.
every time I tried to use those fonts, GIMP crashed.

(*) I copied the fonts in the Linux font directory, but without reading permission
Comment 1 Jehan 2015-04-29 23:18:34 UTC
I confirm the crash on GIMP 2.8.
I made a font non readable, and GIMP crashes with:

---------------------------
(gimp-2.8:17922): Pango-WARNING **: failed to create cairo scaled font, expect ugly output. the offending font is 'VTC Letterer Pro 16'

(gimp-2.8:17922): Pango-WARNING **: font_face status is: out of memory

(gimp-2.8:17922): Pango-WARNING **: scaled_font status is: out of memory

(script-fu:17929): LibGimpBase-WARNING **: script-fu: gimp_wire_read(): error
Segmentation fault
---------------------------

But GIMP master does not crash though. When I use a non-readable font, the letters just become all "unknown character" squares instead.

I'm not sure if we really want to spend time fixing the crash on 2.8 therefore if the bug does not exist on master. This said, it could be interesting to have an error message in master for users to know there are reading problems on a font file (just replacing all letters with squares does not give very meaningful information).
Comment 2 Michael Schumacher 2015-11-22 16:18:32 UTC
*** Bug 758492 has been marked as a duplicate of this bug. ***
Comment 3 Sora Pifaniro 2015-12-01 10:27:01 UTC
Hello,

We are students who would like to work on this bug. If you could give us few suggestions and/or a starting point, it'll be really appreciated.

Thanks for your answer.
Comment 4 Jehan 2015-12-01 14:02:56 UTC
Hello Sora,

A reproducible crash should not be too hard to fix. To diagnose it, simply build GIMP with debug, and run it with `gdb`:

> $ gdb app/.libs/gimp-2.8
> (gdb) run

Then when the crash occurs, run:

> (gdb) backtrace

This will give you the stack trace at the moment of the crash, allowing you to see which exact line in the code it crashes on.

Last time I tested, it did not crash on the master branch, but it may be worth checking again, and maybe provide an error message there or something to warn about unreadable font files.
Comment 5 Sora Pifaniro 2015-12-15 11:08:03 UTC
Hello Jehan,


Thanks for your help, we think that an error message should appear if the value "face" of the function "gimp_font_get_sample_string"is equal to NULL. We'd like to know if there are any particular function in gimp that permits to display error messages. 

Thanks in advance.

-Sora
Comment 6 Jehan 2015-12-15 13:39:51 UTC
Look for `gimp_message()`. You'll find examples in existing code.
Thanks!
Comment 7 Sora Pifaniro 2016-01-14 22:45:47 UTC
Finally, I think that adding a static function like the following in app/text/gimp-fonts.c may be a solution to alert the user.

static void
gimp_fonts_check_read_permissions(Gimp* gimp, GList* fonts_config_path)
{
  GList       *list;
  GDir        *dir;
  gchar       *config_path;
  gchar       *font_fullpath;
  const gchar *font_name;

  for (list = fonts_config_path; list; list = list->next)
    {
      config_path = g_file_get_path((GFile*)list->data);
      dir = g_dir_open(config_path, 0, NULL);
      while ((font_name = g_dir_read_name(dir)))
        {
          font_fullpath = g_strconcat(config_path, "/", font_name, NULL);
          if (g_access(font_fullpath, R_OK))
              gimp_message(gimp, NULL, GIMP_MESSAGE_ERROR,
                           "Cannot read %s", font_fullpath);
          g_free(font_fullpath);
        }
      g_dir_close(dir);
      g_free(config_path);
    }
}

This function should be call in `gimp_fonts_load()` after the `path` variable assignment just like that :

void
gimp_fonts_load (Gimp *gimp)
{
  ...

  fonts_conf = gimp_sysconf_directory_file (CONF_FNAME, NULL);
  if (! gimp_fonts_load_fonts_conf (config, fonts_conf))
    goto cleanup;

  path = gimp_config_path_expand_to_files (gimp->config->font_path, FALSE);
  gimp_fonts_check_read_permissions(gimp, path);
  gimp_fonts_add_directories (config, path);
  g_list_free_full (path, (GDestroyNotify) g_object_unref);

  ...
}

Display the error message with a proper dialog box may also be a good idea but I don't really understand how to do that.

I have tried to use `gimp_message_box_*` functions with `gimp_message()` but it looks like if he can't find gtk even if I put the gtk include line.
Comment 8 liam 2016-01-17 01:30:53 UTC
Did you test that this change fixes the problem? I'd be surprised if it did, although it's not impossible. The code tries to print a message for not-readable fonts but does remove the font from the list I think...

Also, try making a font file unreadable after GIMP has started, and then using the font - does that also crash?

Note, a user might have thousands of installed fonts, so having to click OK on each failure might not be ideal. A console message seems fine to me.
Comment 9 Jehan 2016-01-17 15:31:34 UTC
Liam's remark is quite insightful. If it happened that the user had hundreds of unreadable fonts (could happen that a permission issue happened while copying a whole directory for instance), showing hundreds of popups would not be a good user experience.

The easy solution to this is therefore console warnings, as proposed by Liam. This one though would only be useful for people running GIMP from console (others would be frustrated not understanding why they don't see their fonts).
A better solution would be to still show a popup, but a single one at the end, ideally with the list of all the fonts not loading in some kind of "Show More" expanding box (GtkExpander) with scrolling capabilities (we don't want to have a popup box bigger than the screen if filled with thousands of problematic font paths). I don't think we have such a message box widget. You would need to implement a new widget.

As a last note, your patch seems to focus on permission issues. What if the file is not readable for other reasons (file corruption, etc.)? Your patch would not be able to detect and inform the user of the issue.
To be more generic, it would be better to have a check on actual fontconfig load (so we output an error for whatever fontconfig actually failed to load, whatever the reason is). I see we just add directories as FcConfigAppFontAddDir(), which I guess won't return FALSE if some of the fonts in the directory are readable.
I wonder if the solution could not be to use FcConfigAppFontAddFile(), instead, and loop ourselves through at least user-controlled directories.
Comment 10 Jehan 2016-10-20 17:35:49 UTC
GUI-wise, another interesting alternative to an error popup would be that unreadable fonts are greyed-out in the list of fonts, with a mouse-over text saying something like "Font bla is unreadable".

Adding the "newcomers" keyword since such implementation should not be too hard.
Comment 11 Michael Natterer 2016-10-20 20:30:22 UTC
Um, sorry :) But how i this "not too hard" if GIMP doesn't even touch
the font files itself, it all happens in 3rd party code.
Comment 12 Jehan 2016-10-20 20:50:11 UTC
Mitch > I guess you haven't read the whole discussion. :P

We should be able to get a return of whether a font is not readable on GIMP side and do something about it (warn the user and just remove the font from the list, or keep it but grayed out). We don't have to go to the crash. Also it doesn't even crash anymore on master code anyway.

So the whole discussion here is just: what do we do when a font is not readable to warn in a user-friendly way? And this happens on our code.
Comment 13 Jehan 2018-04-29 15:40:10 UTC
Ok so I fixed the issue, by using exactly the implementation I proposed in comment 9. Basically FcConfigAppFontAddDir() was still loading some unloadable fonts (well it depends. If I removed read permission from user+group+other, then they were unloaded, but removing only from user for instance, they would still loaded while not being actually readable), which ended up in the list of fonts, but were of course unusable (only showing unknown character squares) and were outputting a bunch of warnings:

> (lt-gimp-2.10:23792): Pango-WARNING **: failed to create cairo scaled font, expect ugly output. the offending font is 'id-kana001 14.8798828125'
> 
> (lt-gimp-2.10:23792): Pango-WARNING **: font_face status is: file not found
> 
> (lt-gimp-2.10:23792): Pango-WARNING **: scaled_font status is: file not found
> 
> (lt-gimp-2.10:23792): Pango-WARNING **: shaping failure, expect ugly output. shape-engine='PangoFcShapeEngine', font='id-kana001 14.8798828125', text='aaaaa'

The below commit fixes it. I just loop myself through files (recursively in sub-folders) and use FcConfigAppFontAddFile() on each file.
GIMP will now output on stderr if a font fails to load, which still allows to detect these, while not bothering users with thousands of fonts.

commit 00dcd50a6c70ef5107d83187d6ccfd95d3f84fda (HEAD -> master, origin/master, origin/HEAD)
Author: Jehan <jehan@girinstud.io>
Date:   Sun Apr 29 17:30:01 2018 +0200

    Bug 748553 - GIMP crash if a font does not have read permission.
    
    Do not use FcConfigAppFontAddDir() because it seems to be half-loading
    some fonts, even when they are not actually readable. On the other hand,
    FcConfigAppFontAddFile() properly fails and does not leave the font list
    in unstable state.

 app/text/gimp-fonts.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 10 deletions(-)