GNOME Bugzilla – Bug 709819
Fix a crash if the script fails and the file can’t be read
Last modified: 2013-10-29 19:58:11 UTC
If the thumbnailer script returns a non-0 exit status and then the file being thumbnailed can’t be read (e.g. because it’s a directory), the thumbnailer currently emits a critical error in gdk_pixbuf_loader_close() because it passes a NULL loader. Patch coming up to fix this, plus a few other patches to fix some compiler warnings.
Created attachment 256908 [details] [review] thumbnailer: Remove duplicate function declarations This shuts up a gcc warning.
Created attachment 256909 [details] [review] thumbnailer: Fix a signed/unsigned comparison This shuts up a gcc warning but shouldn’t result in functional changes.
Created attachment 256910 [details] [review] thumbnailer: Add missing cases to switch statement This shuts up a gcc warning and does not introduce functional changes.
Created attachment 256911 [details] [review] thumbnailer: Bail if no pixbuf loader could be created This prevents gdk_pixbuf_loader_close() from emitting a critical error due to being called with a NULL loader. The loader can be NULL if there was an error in the first read from the input stream.
I am not a gnome-desktop reviewer but the patches looks good to me.
Review of attachment 256910 [details] [review]: I don't think gcc should warn about this if you have a default: case
Review of attachment 256908 [details] [review]: sure
Review of attachment 256909 [details] [review]: sure
Review of attachment 256911 [details] [review]: looks ok
Committed all the patches except the case one to master, thanks. commit b2d6cc3542b8b2fa787253bb45c22b5a151ac164 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Thu Oct 10 14:22:35 2013 +0100 thumbnailer: Bail if no pixbuf loader could be created This prevents gdk_pixbuf_loader_close() from emitting a critical error due to being called with a NULL loader. The loader can be NULL if there was an error in the first read from the input stream. https://bugzilla.gnome.org/show_bug.cgi?id=709819 libgnome-desktop/gnome-desktop-thumbnail.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) commit 40ecc92f964540d4475febd96a7663f68fbdc3b1 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Thu Oct 10 14:21:46 2013 +0100 thumbnailer: Fix a signed/unsigned comparison This shuts up a gcc warning but shouldn’t result in functional changes. https://bugzilla.gnome.org/show_bug.cgi?id=709819 libgnome-desktop/gnome-desktop-thumbnail.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) commit 29c1bc757f34f1d0b32bc12fcc682faba05b8509 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Thu Oct 10 14:20:54 2013 +0100 thumbnailer: Remove duplicate function declarations This shuts up a gcc warning. https://bugzilla.gnome.org/show_bug.cgi?id=709819 libgnome-desktop/gnome-desktop-thumbnail.c | 3 --- 1 file changed, 3 deletions(-)
Created attachment 257238 [details] [review] thumbnailer: Add missing cases to switch statement This shuts up a gcc warning by adding support for thumbnail directories being unmounted or moved.
(In reply to comment #6) > Review of attachment 256910 [details] [review]: > > I don't think gcc should warn about this if you have a default: case I find the warning (-Wswitch-enum) useful because it notifies you about cases which have been added to the enum but which you’ve forgotten to handle in the switch statement. Adding them to the switch shows that they’ve been explicitly considered for that code, and not just forgotten. Taking another look at the patch, I guess we should handle MOVED and UNMOUNTED. Here’s a new version which does that.
(In reply to comment #13) > Taking another look at the patch, I guess we should handle MOVED and UNMOUNTED. > Here’s a new version which does that. Ping?
Review of attachment 257238 [details] [review]: I don't know this code well; your patch looks right at least superficially to me. If you've tested it I'm fine with this. ::: libgnome-desktop/gnome-desktop-thumbnail.c @@ +725,3 @@ + + if (!g_str_has_suffix (dirent, THUMBNAILER_EXTENSION)) + return; Leaks dir; I suggest a "goto out"
(In reply to comment #15) > ::: libgnome-desktop/gnome-desktop-thumbnail.c > @@ +725,3 @@ > + > + if (!g_str_has_suffix (dirent, THUMBNAILER_EXTENSION)) > + return; > > Leaks dir; I suggest a "goto out" It should have actually been a ‘continue’ anyway. Changed and pushed to master. commit c064fab680c287bfa6885b56da69c0b8de401dc6 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Thu Oct 10 14:22:12 2013 +0100 thumbnailer: Add missing cases to switch statement This shuts up a gcc warning by adding support for thumbnail directories being unmounted or moved. https://bugzilla.gnome.org/show_bug.cgi?id=709819 libgnome-desktop/gnome-desktop-thumbnail.c | 144 ++++++++++++++++++++++++++----------- 1 file changed, 103 insertions(+), 41 deletions(-)