GNOME Bugzilla – Bug 752824
Zeitgeist plugin read/write after free on datasource registration
Last modified: 2019-03-23 20:35:04 UTC
Created attachment 308077 [details] [review] Patch The Zeitgeist plugin creates a GPtrArray to give to libzeitgeist when registering a Zeitgeist datasource. libzeitgeist internally sinks this reference, but the plugin assumes the GPtrArray to still be available and later attempts to unref it. The attached patch adds a reference to the GPtrArray to keep it available until the end of the plugin's initialisation code, where it is unrefed.
Review of attachment 308077 [details] [review]: It's easier for us if the patch is created with the 'git format-patch' command, with a good commit message. Otherwise, the diff looks good.
Created attachment 308512 [details] [review] Git-formatted patch for gedit-plugins trunk Here you go!
Pushed: https://git.gnome.org/browse/gedit-plugins/commit/?id=741952f0986d5c1bdf8a11f06144010613fe34d2 I had to fix the indentation (!), and the commit message. The first line of the commit message should be short, no longer than 72 characters. See the HACKING file in the main gedit repository (not gedit-plugins).
Wouldn't the patch be simpler just to move the g_ptr_array_set_free_func call before it's passed to zeitgeist_data_source_new_full? Then it wouln't need the ref/unref.
Re-reading the code I'm even more confused about the reference counting. I *think* the reference is just being kept for the PtrArray just to unref the single event object inside it. So you could probably just unref the event after calling zeitgeist_data_source_new_full and that would be simpler.
Sorry it took me so long to get back to you. You're perfectly right, it makes little sense to re-reference the structure rather than simply removing the incorrect unref. The code is semantically equivalent but has dead code. The reason I decided to do it this way is human factors. The person who wrote this code in the first place might have been unfamiliar with libzeitgeist, since she did not realise the called function took ownership of its parameter. That person is likelier than someone else to maintain the code in the future, and this bug has existed for ~4 years and caused multiple reports of crashes without anyone finding out how to fix it. I stumbled upon the issue by pure chance myself, because of my own hacking of libzeitgeist internal state. I chose to make sure the offending object is available wherever the original developer assumed it was so that the bug isn't reintroduced in the future. If you're confident this is unlikely to happen (or that you'll be more prepared to link crash reports to this specific plugin), I will happily agree with you that it's preferable to remove the redundant unref instead. I've just preferred to be overly cautious :-)