On Fri, Oct 02, 2020 at 12:04:21PM +0200, Michal Prívozník wrote: > On 10/2/20 11:52 AM, Pavel Hrdina wrote: > > On Fri, Oct 02, 2020 at 11:22:06AM +0200, Michal Privoznik wrote: > > > The virGDBusCallMethod() and virGDBusCallMethodWithFD() are > > > simple wrappers over g_dbus_connection_call_sync() and > > > g_dbus_connection_call_with_unix_fd_list_sync() respectively. The > > > documentation to these function states that passed parameters > > > (@message in our case) is consumed for 'convenient' inline use of > > > g_variant_new() [1]. But that means we must not unref the message > > > afterwards. To make it explicit that the message is consumed the > > > signature of our wrappers is changed too. > > > > > > 1: https://developer.gnome.org/gio/stable/GDBusConnection.html#g-dbus-connection-call-sync > > > > > > Reported-by: Cole Robinson <crobinso@xxxxxxxxxx> > > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > > > --- > > > src/rpc/virnetdaemon.c | 4 ++-- > > > src/util/virfirewalld.c | 16 ++++++++-------- > > > src/util/virgdbus.c | 29 ++++++++++++++++++++--------- > > > src/util/virgdbus.h | 4 ++-- > > > src/util/virpolkit.c | 4 ++-- > > > src/util/virsystemd.c | 23 ++++++++--------------- > > > 6 files changed, 42 insertions(+), 38 deletions(-) > > > > This doesn't look right, I know about the limitation and was counting > > with it when introducing virgdbus.c where we call g_variant_ref_sink() > > on the passed message. > > > > g_variant_new() returns floating reference which makes it possible to us > > it as described in the g_dbus_connection_call_sync() documentation but > > for our convenience I changed the behavior in virGDBusCallMethod() to > > make it a normal reference so it is not consumed. The motivation was to > > not introduce a new reference concept into libvirt code. > > > > I'll look into the issue but this should not happen. If you look into > > g_dbus_connection_call_sync() code in GLib they call > > g_variant_ref_sink() on the passed data as well and if they receive a > > normal reference they will simply add a new normal reference instead of > > consuming it. > > > > In addition the GLib g_dbus_connection_call_sync() should not be called > > from our tests because we mock this call. > > > > I'll look again into the issue to figure out what's wrong. > > g_variant_ref_sink() does not do what you think. Here's the code: > GVariant * > g_variant_ref_sink (GVariant *value) > { > g_return_val_if_fail (value != NULL, NULL); > g_return_val_if_fail (!g_atomic_ref_count_compare (&value->ref_count, 0), > NULL); > > g_variant_lock (value); > > if (~value->state & STATE_FLOATING) > g_variant_ref (value); > else > value->state &= ~STATE_FLOATING; > > g_variant_unlock (value); > > return value; > } > > > So if the variant has STATE_FLOATING bit set (which it does), it doesn't > increase the refcounter. It just clears it. I know how it works :) Our g_variant_ref_sink() call in virGDBusCallMethod() changes the data from floating reference to normal reference and the same call in g_dbus_connection_call_sync() adds another reference to the data so it looks like this: virGDBusCallMethod(); if (data) g_variant_ref_sink(); g_dbus_connection_call_sync(); if (data) message->body = g_variant_ref_sink (data); Now there are two normal references for data. Later g_dbus_connection_call_sync() frees the message which also removes the second reference from @data and the last reference should be freed by caller of virGDBusCallMethod(). The bug here is in the tests themselves, I'll post a patch in a minute. Pavel
Attachment:
signature.asc
Description: PGP signature
