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.
Michal
