GNOME Bugzilla – Bug 712536
Themes with specified gap_file, gap_start_file or gap_end_file but no actual file crash
Last modified: 2013-12-04 12:11:09 UTC
Created attachment 260034 [details] [review] Patch for master. Hi, A user reported that a theme of his would crash GIMP: https://mail.gnome.org/archives/gimp-developer-list/2013-November/msg00027.html I tested and confirmed that the theme named Flat-Dark would indeed crash GIMP master in my Linux Mint, using gtk-2-24 branch. The problem is that the gtkrc is kind of broken (the user basically used existing gtkrc and messed with them) and several gap image are named but no file can actually be located. When this happens, the associated ThemePixbuf must be destroyed and discarded with a warning, same as we do already for absent background and overlay image. But this was not done. I upload 2 patches which fixes the crash and simply discards absent images. One for master, and one for gtk-2-24.
Created attachment 260035 [details] [review] Patch for gtk-2-24
Review of attachment 260035 [details] [review]: looks good to me. ::: modules/engines/pixbuf/pixbuf-rc-style.c @@ +808,3 @@ } + if (data->gap && !data->gap->filename) minor coding style issue — since you're checking for data->gap->filename in all three conditions, this would probably be easier/more readable: /* filename is required for all gap image options */ if (!data->gap->filename) { if (data->gap) warn_and_unset_gap; if (data->gap_start) warn_and_unset_gap_start; if (data->gap_end) warn_and_unset_gap_end; } even having a small theme_pixbuf_clear() utility function that takes a pointer to NULL-ify would help.
Hi, Thanks for the review. I don't check for data->gap_start->filename in all conditions. I checked for respective data->gap_start->filename and data->gap_end->filename. Every test is different, there are no common points.
Created attachment 260099 [details] [review] pixbuf-engine: Improve ThemePixbuf clean up functions Make theme_pixbuf_destroy() NULL-safe like g_free(), and add a clear function in the spirit of the g_clear_* family of functions.
Created attachment 260100 [details] [review] pixbuf-engine: Clean up error conditions and destructors Simplify the error checks and move all common behaviour into a utility function.
Attachment 260099 [details] pushed as e2aabc0 - pixbuf-engine: Improve ThemePixbuf clean up functions Attachment 260100 [details] pushed as e4c83bb - pixbuf-engine: Clean up error conditions and destructors
Created attachment 261285 [details] [review] Fix factorization bug from commit 34fd123 Hi, Thanks for this. The improvements from Emmanuele look all nice, except for the code factorisation of my code. As I said in comment 3: "I don't check for data->gap_start->filename in all conditions. I checked for respective data->gap_start->filename and data->gap_end->filename. Every test is different, there are no common points." So the changes of attachment 260100 [details] [review] partly reintroduces the bug I fixed with my initial commit! For instance a gap_start_file and gap_end_file in a theme defined with no corresponding file would still crash a GTK+ application! I tested: the crash I fixed reappeared and GIMP crashed on a broken gtkrc. Worst by changing the order, you introduced a new bug. If no gap_file was defined, testing data->gap->filename is obviously wrong too. Order mattered. You have to test data->gap before data->gap->filename. So I persist: + if (data->gap && !data->gap->filename) + { + } + + if (data->gap_start && !data->gap_start->filename) + { + } + + if (data->gap_end && !data->gap_end->filename) + { + } And: if (!data->gap->filename) { if (data->gap) warn_and_unset_gap; if (data->gap_start) warn_and_unset_gap_start; if (data->gap_end) warn_and_unset_gap_end; } are completely different tests! :-) So I must reopen the bug report, sorry for this, but the crash is back. Attached the patch to fix the bug again. Can I push to master and gtk-2-24? Thanks.
Reopened.
Created attachment 263501 [details] [review] Update "Fix factorization bug from commit 34fd123" Since the code has been touched (https://git.gnome.org/browse/gtk+/commit/?h=gtk-2-24&id=9559c707d59c5e5c34596f8f75c7ed0b97b4a196), I update my patch.
Review of attachment 263501 [details] [review]: right
Pushed on master and gtk-2-24: commit 4e4c248077f24add1ccf133a54585434461d680f Author: Jehan <jehan@girinstud.io> Date: Sat Nov 23 18:35:18 2013 +1300 Bug 712536 - Themes with gap_(start|end)_file but no actual file crash Code factorization in commit 34fd123 reintroduced bug fixed in 0d396ab with non-equivalent factorized tests.