Bug 99741 - Audit and fix GVariant reference counting
Summary: Audit and fix GVariant reference counting
Status: RESOLVED FIXED
Alias: None
Product: PolicyKit
Classification: Unclassified
Component: daemon (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: David Zeuthen (not reading bugmail)
QA Contact: David Zeuthen (not reading bugmail)
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-09 19:31 UTC by Miloslav Trmac
Modified: 2018-04-03 18:26 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
0001-Simplify-GVariant-reference-counting.patch (21.24 KB, patch)
2017-02-09 19:34 UTC, Miloslav Trmac
Details | Splinter Review
0002-Fix-a-memory-leak-on-an-error-path-of-lookup_asv-twi.patch (1.46 KB, patch)
2017-02-09 19:34 UTC, Miloslav Trmac
Details | Splinter Review
0003-Fix-a-memory-leak-in-server_handle_register_authenti.patch (1.05 KB, patch)
2017-02-09 19:34 UTC, Miloslav Trmac
Details | Splinter Review
0004-Fix-a-memory-leak-in-server_handle_unregister_authen.patch (1.03 KB, patch)
2017-02-09 19:34 UTC, Miloslav Trmac
Details | Splinter Review
0005-Fix-a-memory-leak-in-server_handle_authentication_ag.patch (1.32 KB, patch)
2017-02-09 19:34 UTC, Miloslav Trmac
Details | Splinter Review
0006-Fix-memory-leaks-in-server_handle_-_temporary_author.patch (1.33 KB, patch)
2017-02-09 19:35 UTC, Miloslav Trmac
Details | Splinter Review
0007-Fix-error-handling-in-polkit_authority_enumerate_tem.patch (1.31 KB, patch)
2017-02-09 19:35 UTC, Miloslav Trmac
Details | Splinter Review
0008-Fix-a-memory-leak-per-agent-authentication.patch (1.69 KB, patch)
2017-02-09 19:35 UTC, Miloslav Trmac
Details | Splinter Review
0009-Fix-a-memory-leak-on-agent-authentication-cancellati.patch (1.46 KB, patch)
2017-02-09 19:35 UTC, Miloslav Trmac
Details | Splinter Review
0001-polkitpermission-Fix-a-memory-leak-on-authority-chan.patch (780 bytes, patch)
2017-03-02 14:01 UTC, Rui Tiago Matos
Details | Splinter Review
0001-Simplify-GVariant-reference-counting.patch (21.98 KB, patch)
2017-06-21 20:51 UTC, Miloslav Trmac
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Miloslav Trmac 2017-02-09 19:31:20 UTC
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.
Comment 1 Miloslav Trmac 2017-02-09 19:34:01 UTC
Created attachment 129448 [details] [review]
0001-Simplify-GVariant-reference-counting.patch
Comment 2 Miloslav Trmac 2017-02-09 19:34:13 UTC
Created attachment 129449 [details] [review]
0002-Fix-a-memory-leak-on-an-error-path-of-lookup_asv-twi.patch
Comment 3 Miloslav Trmac 2017-02-09 19:34:34 UTC
Created attachment 129450 [details] [review]
0003-Fix-a-memory-leak-in-server_handle_register_authenti.patch
Comment 4 Miloslav Trmac 2017-02-09 19:34:44 UTC
Created attachment 129451 [details] [review]
0004-Fix-a-memory-leak-in-server_handle_unregister_authen.patch
Comment 5 Miloslav Trmac 2017-02-09 19:34:56 UTC
Created attachment 129452 [details] [review]
0005-Fix-a-memory-leak-in-server_handle_authentication_ag.patch
Comment 6 Miloslav Trmac 2017-02-09 19:35:08 UTC
Created attachment 129453 [details] [review]
0006-Fix-memory-leaks-in-server_handle_-_temporary_author.patch
Comment 7 Miloslav Trmac 2017-02-09 19:35:22 UTC
Created attachment 129454 [details] [review]
0007-Fix-error-handling-in-polkit_authority_enumerate_tem.patch
Comment 8 Miloslav Trmac 2017-02-09 19:35:36 UTC
Created attachment 129455 [details] [review]
0008-Fix-a-memory-leak-per-agent-authentication.patch
Comment 9 Miloslav Trmac 2017-02-09 19:35:47 UTC
Created attachment 129456 [details] [review]
0009-Fix-a-memory-leak-on-agent-authentication-cancellati.patch
Comment 10 Colin Walters 2017-02-21 17:12:19 UTC
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.
Comment 11 Miloslav Trmac 2017-02-21 17:30:55 UTC
*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…
Comment 12 Rui Tiago Matos 2017-03-02 14:01:36 UTC
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)
Comment 13 Colin Walters 2017-03-04 19:05:48 UTC
@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.
Comment 14 Colin Walters 2017-03-04 19:07:40 UTC
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 15 Miloslav Trmac 2017-04-04 21:09:12 UTC
Comment on attachment 130032 [details] [review]
0001-polkitpermission-Fix-a-memory-leak-on-authority-chan.patch

Review of attachment 130032 [details] [review]:
-----------------------------------------------------------------

ACK, thanks!
Comment 16 Miloslav Trmac 2017-04-04 21:10:35 UTC
(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.)
Comment 17 Miloslav Trmac 2017-06-21 20:34:04 UTC
(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).
Comment 18 Miloslav Trmac 2017-06-21 20:51:24 UTC
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.
Comment 19 Ray Strode [halfline] 2018-04-03 18:26:27 UTC
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.