This patch series fixes a fair number of memory leaks revolving around GVariant memory allocation and D-Bus calls (did I ever mention that C is a horrible language for data manipulation and IPC?). Colin, could you review this carefully? Sadly we don't have a good enough test harness to reach all of the code paths, and I can’t commit to building it now. The first patch in the series does not actually fix any leaks, only simplifies the code; the rest are leak fixes, one or two per patch.
Created attachment 129448 [details] [review] 0001-Simplify-GVariant-reference-counting.patch
Created attachment 129449 [details] [review] 0002-Fix-a-memory-leak-on-an-error-path-of-lookup_asv-twi.patch
Created attachment 129450 [details] [review] 0003-Fix-a-memory-leak-in-server_handle_register_authenti.patch
Created attachment 129451 [details] [review] 0004-Fix-a-memory-leak-in-server_handle_unregister_authen.patch
Created attachment 129452 [details] [review] 0005-Fix-a-memory-leak-in-server_handle_authentication_ag.patch
Created attachment 129453 [details] [review] 0006-Fix-memory-leaks-in-server_handle_-_temporary_author.patch
Created attachment 129454 [details] [review] 0007-Fix-error-handling-in-polkit_authority_enumerate_tem.patch
Created attachment 129455 [details] [review] 0008-Fix-a-memory-leak-per-agent-authentication.patch
Created attachment 129456 [details] [review] 0009-Fix-a-memory-leak-on-agent-authentication-cancellati.patch
I think we should start using the [GLib autocleanup](https://github.com/GNOME/glib/blob/master/glib/glib-autocleanups.h) macros. I basically can't imagine writing C without them anymore. Not sure offhand if any target we care about is using a too-old GLib for them. Probably. We can vendor in the bits like has been done for https://github.com/GNOME/libglnx/blob/master/glnx-backport-autocleanups.h which is used by ostree/flatpak at least. I think NM has a copy of that instead of using it as a submodule.
*shrug* I can live with g_autoptr(), and update this patchset for it (but absolutely not promising to do a wholescale conversion). I am, though, very uninterested in maintaining a fork of the g_autoptr() functionality inside polkit. g_autoptr seems to come from 2.44, which seems old enough, given the recent mozjs API bump we are forcing the users to have a somewhat updated system anyway. The last attempt to bump glib versions got stalled on https://bugs.freedesktop.org/show_bug.cgi?id=91198 , I guess I should follow up there…
Created attachment 130032 [details] [review] 0001-polkitpermission-Fix-a-memory-leak-on-authority-chan.patch Here's one more leak I found looking at a gnome-shell valgrind log: ==00:17:55:50.737 19386== 145,720,696 (22,854,560 direct, 122,866,136 indirect) bytes in 571,364 blocks are definitely lost in loss record 47,122 of 47,122 ==00:17:55:50.737 19386== at 0x4C29BFD: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==00:17:55:50.737 19386== by 0xE40830D: g_malloc (gmem.c:94) ==00:17:55:50.737 19386== by 0xE41E0FD: g_slice_alloc (gslice.c:1007) ==00:17:55:50.737 19386== by 0xE41E63D: g_slice_alloc0 (gslice.c:1032) ==00:17:55:50.737 19386== by 0xE199F56: g_type_create_instance (gtype.c:1856) ==00:17:55:50.737 19386== by 0xE17DD2C: g_object_new_internal (gobject.c:1779) ==00:17:55:50.737 19386== by 0xE17F8EC: g_object_newv (gobject.c:1926) ==00:17:55:50.737 19386== by 0xE1800B3: g_object_new (gobject.c:1619) ==00:17:55:50.737 19386== by 0x892A893: polkit_authorization_result_new (polkitauthorizationresult.c:111) ==00:17:55:50.737 19386== by 0x892ABB7: polkit_authorization_result_new_for_gvariant (polkitauthorizationresult.c:287) ==00:17:55:50.737 19386== by 0x8922CB0: check_authorization_cb (polkitauthority.c:829) ==00:17:55:50.737 19386== by 0xD2C0F92: g_task_return_now (gtask.c:1106) ==00:17:55:50.737 19386== by 0xD2C162D: g_task_return (gtask.c:1164) ==00:17:55:50.737 19386== by 0xD31A28A: reply_cb (gdbusproxy.c:2580) ==00:17:55:50.737 19386== by 0xD2C0F92: g_task_return_now (gtask.c:1106) ==00:17:55:50.737 19386== by 0xD2C162D: g_task_return (gtask.c:1164) ==00:17:55:50.737 19386== by 0xD30ECE9: g_dbus_connection_call_done (gdbusconnection.c:5704) ==00:17:55:50.737 19386== by 0xD2C0F92: g_task_return_now (gtask.c:1106) ==00:17:55:50.737 19386== by 0xD2C0FC8: complete_in_idle_cb (gtask.c:1120) ==00:17:55:50.737 19386== by 0xE402D79: g_main_dispatch (gmain.c:3152) ==00:17:55:50.737 19386== by 0xE402D79: g_main_context_dispatch (gmain.c:3767) ==00:17:55:50.737 19386== by 0xE4030B7: g_main_context_iterate.isra.24 (gmain.c:3838) ==00:17:55:50.737 19386== by 0xE403389: g_main_loop_run (gmain.c:4032) ==00:17:55:50.737 19386== by 0xA065F37: meta_run (main.c:473) ==00:17:55:50.737 19386== by 0x402454: main (main.c:471)
@mitr it looks like you're using tabs for indents in places, and it makes things look off here (Spacemacs, infers tab-width=2 I guess?). AFAICS most of the code is only using spaces, which is expected for GNU style.
OK to push? ``` From d5a6dc92feeddc432110cb9c8ebac391186a749a Mon Sep 17 00:00:00 2001 From: Colin Walters <walters@verbum.org> Date: Sat, 4 Mar 2017 14:06:49 -0500 Subject: [PATCH] Add .editorconfig and .dir-locals.el Cargo culting these from github:projectatomic/bubblewrap to help contributor's editors do the right thing. --- .dir-locals.el | 2 +- .editorconfig | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 .editorconfig diff --git a/.dir-locals.el b/.dir-locals.el index abf24af..3514040 100644 --- a/.dir-locals.el +++ b/.dir-locals.el @@ -1 +1 @@ -((c-mode . ((indent-tabs-mode . nil))))) +((c-mode . ((indent-tabs-mode . nil) (c-file-style . "gnu")))) diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 0000000..d062d2c --- /dev/null +++ b/.editorconfig @@ -0,0 +1,6 @@ +[*.[ch]] +indent_style = space +indent_size = 2 +trim_trailing_whitespace = true +indent_brace_style = gnu + -- 2.9.3 ```
Comment on attachment 130032 [details] [review] 0001-polkitpermission-Fix-a-memory-leak-on-authority-chan.patch Review of attachment 130032 [details] [review]: ----------------------------------------------------------------- ACK, thanks!
(In reply to Rui Tiago Matos from comment #12) > Created attachment 130032 [details] [review] [review] > 0001-polkitpermission-Fix-a-memory-leak-on-authority-chan.patch Thanks, committed as df6488c0a5b2a6c7a2d4f6a55008263635c5571b. (I’ll mark the patch as obsolete to make it easier to manage the outstanding patches.)
(In reply to Colin Walters from comment #14) > OK to push? > > ``` > From d5a6dc92feeddc432110cb9c8ebac391186a749a Mon Sep 17 00:00:00 2001 > From: Colin Walters <walters@verbum.org> > Date: Sat, 4 Mar 2017 14:06:49 -0500 > Subject: [PATCH] Add .editorconfig and .dir-locals.el Sure (though in my case it was Eclipse).
Created attachment 132124 [details] [review] 0001-Simplify-GVariant-reference-counting.patch (In reply to Colin Walters from comment #13) > @mitr it looks like you're using tabs for indents in places, and it makes > things look off here (Spacemacs, infers tab-width=2 I guess?). AFAICS most > of the code is only using spaces, which is expected for GNU style. (I don't see that codified in https://www.gnu.org/prep/standards/html_node/Formatting.html#Formatting , but) sure, that makes sense. This affected only patch 0001, this is an updated version.
Pushing these now. They look good and we can g_auto stuff later.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.