GNOME Bugzilla – Bug 705124
Memory leak in some vfuncs
Last modified: 2015-03-17 14:10:27 UTC
Some virtual functions in glib (perhaps also in other packages) return a const pointer to an object that the caller shall not free or unref, because it belongs to the callee. The examples I have found in glib/gio that are wrapped with _WRAP_VFUNC are GActionGroup const GVariantType* get_action_parameter_type() const GVariantType* get_action_state_type() GAction const gchar* get_name() const GVariantType* get_parameter_type() const GVariantType* get_state_type() GDBusObject const gchar* get_object_path() TlsPassword const guchar* get_value() No leak! const gchar* get_default_warning() Take GAction::get_name() as an example. #m4 _CONVERSION(`Glib::ustring',`const gchar*',`g_strdup($3.c_str())') _WRAP_VFUNC(Glib::ustring get_name() const, "get_name") Part of the generated code: const gchar* Action_Class::get_name_vfunc_callback(GAction* self) { ..... if(obj_base && obj_base->is_derived_()) { ...... return g_strdup(obj->get_name_vfunc().c_str()); ...... } ...... } Glib::ustring Gio::Action::get_name_vfunc() const { ...... if(base && base->get_name) { Glib::ustring retval(Glib::convert_const_gchar_ptr_to_ustring( (*base->get_name)(const_cast<GAction*>(gobj())))); return retval; } ...... } Glib::convert_const_gchar_ptr_to_ustring() creates a Glib::ustring with a copy of the string returned from base->get_name(). Correct! That copy is deleted eventually. g_strdup(obj->get_name_vfunc().c_str()) creates a copy of the string returned by obj->get_name_vfunc(). Necessary but problematic! The string must exist after get_name_vfunc_callback() has returned, but the caller thinks that it belongs to the callee, and will not delete it. Memory leak! I don't know how to fix this problem without breaking ABI or API. The wrapper of TlsPassword::get_value() avoids the problem by not converting from const guchar* to Glib::ustring and back. Similar solutions would be possible for the other functions, but it would not be elegant. E.g. _WRAP_VFUNC(const gchar* get_name() const, "get_name") instead of _WRAP_VFUNC(Glib::ustring get_name() const, "get_name") A possible ABI-breaking solution: Add a private data member, Glib::ustring name, to Gio::Action. In Action_Class::get_name_vfunc_callback(): obj->name = obj->get_name_vfunc(); return obj->name.c_str();
I wonder, how do C developers ensure that the string exists for long enough to be copied, while also making sure to free the string soon enough. I also vaguely remember reading something obscure about how the result of cstr() might stay alive just long enough to be copied by a calling function.
(In reply to comment #1) > I wonder, how do C developers ensure that the string exists for long enough to > be copied, while also making sure to free the string soon enough. These vfuncs in glib return a pointer to object data. The leaking vfuncs in glibmm return a copy of something (probably in most cases also object data). Once again, take GAction::get_name() as an example. GSimpleAction implements the GAction interface. From gsimpleaction.c: struct _GSimpleAction { GObject parent_instance; gchar *name; ...... }; void g_simple_action_iface_init (GActionInterface *iface) { iface->get_name = g_simple_action_get_name; ...... } static const gchar * g_simple_action_get_name (GAction *action) { GSimpleAction *simple = G_SIMPLE_ACTION (action); return simple->name; } The solution in the last paragraph of comment 0 breaks ABI, unless we are willing to introduce an ugly std::map<const Glib::Action*, ExtraActionData> extra_action_data similar to what I've done in Glib::Source (bug 561885) and Glib::ObjectBase (bug 697229). I don't like those maps. They are a last resort. Why don't we use the pimpl idiom (pointer to implementation) in many of our classes? http://en.wikipedia.org/wiki/Pimpl The class data would contain a pointer to private data. A separate struct with the private data would be defined in the .cc file. It could be changed at any time without breaking ABI. We could add or remove private data at any time. > I also vaguely remember reading something obscure about how the result of > cstr() might stay alive just long enough to be copied by a calling function. Bjarne Stroustrup says in "The C++ Programming Language" 3rd edition, about both std::string::c_str() and std::string::data(): "The user also cannot rely on its value after a subsequent call on a non-const function on the string." The 4th edition contains much less information about c_str().
> The class data would contain a pointer to private data. Should be: The instance data would contain a pointer to private data.
Created attachment 298957 [details] [review] patch: Gio::Action: Fix memory leaks in vfuncs From comment 0: > I don't know how to fix this problem without breaking ABI or API. Now I know. Access new private data via a GQuark. The attached patch shows how it can be done for Gio::Action. If you want to test it, use valgrind on glibmm/tests/glibmm_interface_implementation. Is this an acceptable solution? It requires a GQuark for each class with leaking vfuncs. Four quarks in glibmm. Probably some in gtkmm. From comment 2: > The solution in the last paragraph of comment 0 breaks ABI, unless we are > willing to introduce an ugly > std::map<const Gio::Action*, ExtraActionData> extra_action_data > similar to what I've done in Glib::Source (bug 561885) and Glib::ObjectBase > (bug 697229). GQuark can't be used in those two cases. GSource is not a GObject. The new data in Glib::ObjectBase must be stored before a GObject has been created.
Thanks. Wouldn't it be easier to generalize this, and maybe then generate the code, if each vfunc had its own data and quark?
Created attachment 299158 [details] [review] patch: gmmproc: _WRAP_VFUNC: Add the keep_return parameter You are perfectly right. This is much better. In the worst case this requires more quarks, but in most cases (most application programs) it probably does not. Each quark is local static data in a callback function. It's created only if it's actually used. If you don't like the new parameter name keep_return and the new template function name callback_delete_data(), please suggest other names. The template function should have been added long ago. I've found hand- coded functions in several .ccg files that can be replaced by instantiations of callback_delete_data().
Created attachment 299159 [details] [review] patch: Gio::Action etc: Add keep_return to some _WRAP_VFUNC() This is how Gio::Action, ActionGroup, GDBusObject and TlsPassword shall be modified, when it's possible to wright _WRAP_VFUNC(..., keep_return).
Comment 7: s/wright/write/ There's another problem with some vfuncs in Gio::Action and ActionGroup. GAction::get_state_hint(), GAction::get_state(), GActionGroup:: get_action_state_hint(), GActionGroup::get_action_state() return a GVariant*, and the caller gets its own reference. That's done with a call to Glib::VariantBase::gobj_copy(). The problem is that the GVariant* can be 0. I.e. Glib::VariantBase::gobj() == 0. This is allowed according to the glib documentation. See e.g. g_action_get_state(). If Glib::VariantBase::gobj() == 0, Glib::VariantBase::gobj_copy() prints a critical message, although it's no error in this case. I suspect that it's unwise to change gobj_copy(). In other cases a null pointer may be an error, and a critical message appropriate. How about adding a Glib::VariantBase::gobj_copy_or_null()? An alternative would be to add template functions to wrap.h. I think this is a better solution. But once again a better function name is welcome. template <typename T> inline typename T::BaseObjectType* unwrap_copy_or_null(T* ptr) { return (ptr && ptr->gobj()) ? ptr->gobj_copy() : 0; } template <typename T> inline const typename T::BaseObjectType* unwrap_copy_or_null(const T* ptr) { return (ptr && ptr->gobj()) ? ptr->gobj_copy() : 0; } template <typename T> inline typename T::BaseObjectType* unwrap_copy_or_null(const Glib::RefPtr<T>& ptr) { return (ptr && ptr->gobj()) ? ptr->gobj_copy() : 0; } template <typename T> inline const typename T::BaseObjectType* unwrap_copy_or_null(const Glib::RefPtr<const T>& ptr) { return (ptr && ptr->gobj()) ? ptr->gobj_copy() : 0; }
Created attachment 299345 [details] [review] patch: Gio::Action, ActionGroup: Fix critical messages from vfuncs The bug discussed in comment 8 can be fixed more easily. Add template <class T> inline typename T::BaseObjectType* unwrap_copy(const T& obj) { return obj.gobj() ? obj.gobj_copy() : 0; } Then the _WRAP_VFUNC parameter refreturn_ctype can be used in the vfuncs that return a GVariant*, and give the caller a reference.
I have pushed all 3 patches, but I changed the function name Glib::callback_delete_data() to Glib::destroy_notify_delete(). It's intended for glib/gtk+ functions that take a GDestroyNotify parameter. https://git.gnome.org/browse/glibmm/commit/?id=d1f1eca2d3a5044d6b31c4acf49879fda5306fd8 https://git.gnome.org/browse/glibmm/commit/?id=010a2d71e4379a12849d2cda742dfdc0a868d043 https://git.gnome.org/browse/glibmm/commit/?id=b28bc49e7af2b90d23bb4ea460ca15c8eaf0cc54
I have added some tests to tests/glibmm_interface_implementation. It can now be used for testing the fixes in this bug. To test that no memory leaks, run the test program under valgrind. https://git.gnome.org/browse/glibmm/commit/?id=ffc7d974949c8ceb0273a11c649c6af947afcf18 This gtkmm-documentation patch adds a description a _WRAP_VFUNC's new keep_return parameter. https://git.gnome.org/browse/gtkmm-documentation/commit/?id=0fe3c41e81503d113806bb45a0826732c267f240