[PATCH 1/5] lib: Don't unref message passed to virGDBusCallMethod{WithFD}()

[Pavel Hrdina]

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 at redhat.com>
> > > Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> > > ---
> > >   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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201002/4eabb14f/attachment-0001.sig>