[PATCH] desktop-shell: don't segfault on invalid icon path

[Pekka Paalanen]

On Tue, 28 Aug 2012 19:48:12 +0200
Philipp Brüschweiler <blei42 at gmail.com> wrote:

> Instead load a fallback icon and proceed as normal.
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=53860
> 
> v2: coding style fixes
> ---
>  clients/desktop-shell.c | 20 +++++++++++++++++++-
>  1 Datei geändert, 19 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)
> 
> diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c
> index dc87e75..c2d158d 100644
> --- a/clients/desktop-shell.c
> +++ b/clients/desktop-shell.c
> @@ -472,6 +472,24 @@ panel_create(struct display *display)
>  	return panel;
>  }
>  
> +static cairo_surface_t *
> +load_icon_or_fallback(const char *icon)
> +{
> +	cairo_surface_t *surface = cairo_image_surface_create_from_png(icon);
> +
> +	if (cairo_surface_status(surface) == CAIRO_STATUS_SUCCESS)
> +		return surface;
> +
> +	fprintf(stderr, "ERROR loading icon from file '%s'\n", icon);

Should surface be properly destroyed here, before re-creating?

> +	/* loading fallback */
> +	surface =
> +		cairo_image_surface_create_from_png(DATADIR
> +			"/weston/icon_window.png");
> +
> +	return surface;
> +}
> +
>  static void
>  panel_add_launcher(struct panel *panel, const char *icon, const char *path)
>  {
> @@ -481,7 +499,7 @@ panel_add_launcher(struct panel *panel, const char *icon, const char *path)
>  
>  	launcher = malloc(sizeof *launcher);
>  	memset(launcher, 0, sizeof *launcher);
> -	launcher->icon = cairo_image_surface_create_from_png(icon);
> +	launcher->icon = load_icon_or_fallback(icon);
>  	launcher->path = strdup(path);
>  
>  	wl_array_init(&launcher->envp);

Hi Philipp,

thanks for the patch, these little things often get overlooked.

You didn't really fix the segfault, you just made it more unlikely. It
is possible the fallback icon is not there, either. Could you check for
the failure and then just not add the launcher at all, and complain?

Another thought, if launcher->icon is bad, you could use cairo to draw
a box with an X or something. That way you're not depending on any
image files, the launcher item will still work, and the problem will
be obvious.


Thanks,
pq