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 712536 - Themes with specified gap_file, gap_start_file or gap_end_file but no actual file crash
Themes with specified gap_file, gap_start_file or gap_end_file but no actual ...
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: .General
3.11.x
Other All
: Normal critical
: ---
Assigned To: Jehan
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2013-11-17 12:00 UTC by Jehan
Modified: 2013-12-04 12:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for master. (1.38 KB, patch)
2013-11-17 12:00 UTC, Jehan
committed Details | Review
Patch for gtk-2-24 (1.38 KB, patch)
2013-11-17 12:01 UTC, Jehan
committed Details | Review
pixbuf-engine: Improve ThemePixbuf clean up functions (1.97 KB, patch)
2013-11-18 11:07 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
pixbuf-engine: Clean up error conditions and destructors (3.67 KB, patch)
2013-11-18 11:07 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
Fix factorization bug from commit 34fd123 (1.87 KB, patch)
2013-11-23 05:44 UTC, Jehan
none Details | Review
Update "Fix factorization bug from commit 34fd123" (1.79 KB, patch)
2013-12-04 11:45 UTC, Jehan
accepted-commit_now Details | Review

Description Jehan 2013-11-17 12:00:29 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.
Comment 1 Jehan 2013-11-17 12:01:55 UTC
Created attachment 260035 [details] [review]
Patch for gtk-2-24
Comment 2 Emmanuele Bassi (:ebassi) 2013-11-18 10:46:35 UTC
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.
Comment 3 Jehan 2013-11-18 10:52:14 UTC
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.
Comment 4 Emmanuele Bassi (:ebassi) 2013-11-18 11:07:17 UTC
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.
Comment 5 Emmanuele Bassi (:ebassi) 2013-11-18 11:07:27 UTC
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.
Comment 6 Matthias Clasen 2013-11-23 03:20:50 UTC
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
Comment 7 Jehan 2013-11-23 05:44:12 UTC
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.
Comment 8 Jehan 2013-11-23 05:45:06 UTC
Reopened.
Comment 9 Jehan 2013-12-04 11:45:50 UTC
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.
Comment 10 Matthias Clasen 2013-12-04 11:47:33 UTC
Review of attachment 263501 [details] [review]:

right
Comment 11 Jehan 2013-12-04 12:11:09 UTC
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.