GNOME Bugzilla – Bug 650606
memory leaks
Last modified: 2019-02-22 11:46:12 UTC
These leaks are in the same program file. Perhaps they should be tracked as one bug. https://bugs.launchpad.net/ubuntu/+source/libgnome-keyring/+bug/784369 https://bugs.launchpad.net/ubuntu/+source/libgnome-keyring/+bug/784788
Created attachment 188145 [details] [review] patch to fix both leaks * Eliminate early outs from find_items_1_reply * callers of make_attribute_list_va clean up attributes list
Created attachment 188150 [details] [review] fix additional related issue in find_unlocked_1_reply * eliminate early outs from find_unlocked_1_reply
Thanks for catching those leaks. I committed a slightly different version of the patch to the gnome-3-0 branch. I made the patch simpler since I'd like to include this in the 3.0.2 release, and this is a stable branch. So a goal is having the changes be easier to verify. In addition, the second patch seems to be unnecessary. Please correct me if I'm wrong. http://git.gnome.org/browse/libgnome-keyring/commit/?h=gnome-3-0&id=b49e32b32d4d7c326c3bd7dd44ca1c35944a94c4
> the second patch seems to be unnecessary. I guess you are not convinced by my argument that it dbus_message_get_args could end up allocating something in the case when it returns an error. It looks to me like the underlying implementation in _dbus_message_iter_get_args_valist iterates over the va_list, allocating something to each argument one at a time. If there is an error processing one of the arguments, the function jumps out of the iteration over the va_list without cleaning up anything that was allocated for the prior arguments. So I think you have to set each argument to NULL before the call and then make sure you free each one even if there was an error. I don't see any other way to ensure they get cleaned up properly, and I don't think you can call this a bug dbus_message_get_args, because the documentation doesn't promise anything about what happens in the error case. For reference, the version of the dbus message code I'm looking at is here: http://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-message.c
@Stef, commit b49e32b32d4d7c326c3bd7dd44ca1c35944a94c4 crashes nm-connection-editor. https://bugs.archlinux.org/task/24392?getfile=6936
Created attachment 188357 [details] [review] Patch which fixes free() 'invalid pointer' Ionut: Does this patch fix the problem in nm-connection-editor?
(In reply to comment #4) > I guess you are not convinced by my argument that it dbus_message_get_args > could end up allocating something in the case when it returns an error. It > looks to me like the underlying implementation in > _dbus_message_iter_get_args_valist iterates over the va_list, allocating > something to each argument one at a time. landijk-user@yahoo.com: Functions should not allocate out parameters if they return an error. Obviously there are corner cases where this is not the case (such as precondition failures). If libdbus is incorrectly returning allocated memory when dbus_message_get_args returns an error, then you should file a bug with libdbus.
Created attachment 188358 [details] [review] The right patch Sorry, uploaded the wrong patch last time. This should fix the invalid free.
still crashes. backtrace https://bugs.archlinux.org/task/24392#comment77434
Created attachment 188366 [details] [review] Updated patch after seeing backtrace
Seems to be resolved with this patch.
Martin Pitt fixed the problem in bug #650840.
Regarding the error case for dbus_message_get_args, a bug was filed more than two years ago. https://bugs.freedesktop.org/show_bug.cgi?id=21259
(In reply to comment #13) > Regarding the error case for dbus_message_get_args, a bug was filed more than > two years ago. Hmmm, yes. If it interests you, that would be a good place to post a patch fixing the problem.