Re: [PATCH 6/8] qio: Hoist ref of listener outside loop

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Nov 11, 2025 at 02:07:50PM -0600, Eric Blake wrote:
> On Tue, Nov 11, 2025 at 01:09:24PM -0600, Eric Blake wrote:
> > > In the current code, when we unref() the GSource for the socket
> > > watch, the destroy-notify does not get called, because the
> > > event thread is in the middle of a dispatch callback for the
> > > I/O event.  When the dispatch callback returns control to the
> > > event loop, the GSourceCallback is unrefed, and this triggers
> > > the destroy-notify call, which unrefs the listener.
> > > 
> > > The flow looks like this:
> > > 
> > >   Thread 1:
> > >        qio_net_listener_set_client_func(lstnr, f, ...);
> > >            => foreach sock: socket
> > >                => object_ref(lstnr)
> > >                => sock_src = qio_channel_socket_add_watch_source(sock, ...., lstnr, object_unref);
> > > 
> > >   Thread 2:
> > >        poll()
> > >           => event POLLIN on socket
> > >                => ref(GSourceCallback)
> > >                => call dispatch(sock)
> > >                     ...do stuff..
> > > 
> > >   Thread 1:
> > >        qio_net_listener_set_client_func(lstnr, NULL, ...);
> > >           => foreach sock: socket
> > >                => g_source_unref(sock_src)
> > >        unref(lstnr)  (the final reference)
> > >           => finalize(lstnr)
> > 
> > If I understand correctly, _this_ unref(lstnr) is NOT the final
> > reference, because of the additional reference still owned by the
> > GSource that is still dispatching, so finalize(lstnr) is not reached
> > here...
> > 
> > > 
> > >   Thread 2:
> > >               => return dispatch(sock)
> > >               => unref(GSourceCallback)
> > >                   => destroy-notify
> > >                      => object_unref
> > 
> > ...but instead here.  And that is the desirable property of the
> > pre-patch behavior with a per-GSource reference on the listsner object
> > - we are guaranteed that listener can't be finalized while there are
> > any pending dispatch in flight, even if the caller has unref'd their
> > last mention of lstnr, and therefore the dispatch never has a
> > use-after-free.
> 
> But thinking further, even if it is not an object_unref, but merely a
> change of the async callback function, is this the sort of thing where
> a mutex is needed to make things safer?
> 
> Even without my patch series, we have this scenario:
> 
>   Thread 1:
>        qio_net_listener_set_client_func(lstnr, f1, ...);
>            => foreach sock: socket
>                => object_ref(lstnr)
>                => sock_src = qio_channel_socket_add_watch_source(sock, ...., lstnr, object_unref);
> 
>   Thread 2:
>        poll()
>           => event POLLIN on socket
>                => ref(GSourceCallback) // while lstnr->io_func is f1
>                     ...do stuff..
> 
>   Thread 1:
>        qio_net_listener_set_client_func(lstnr, f2, ...);
>           => foreach sock: socket
>                => g_source_unref(sock_src)
>           => foreach sock: socket
>                => object_ref(lstnr)
>                => sock_src = qio_channel_socket_add_watch_source(sock, ...., lstnr, object_unref);
> 
>   Thread 2:
>                => access lstnr->io_func // now sees f2
>                => call f2(sock)
>                => return dispatch(sock)
>                => unref(GSourceCallback)
>                   => destroy-notify
>                      => object_unref
> 
> So even though thread 2 noticed a client while f1 was registered,
> because we lack a mutex and are not using atomic access primitives,
> there is nothing preventing thread 1 from changing the io_func from f1
> to f2 before thread 2 finally calls the callback.  And if f2 is NULL
> (because the caller is deregistering the callback), I could see the
> race resulting in qio_net_listener_channel_func() with its pre-patch
> code of:
> 
>     if (listener->io_func) {
>         listener->io_func(listener, sioc, listener->io_data);
>     }
> 
> resulting in a NULL-pointer deref if thread 1 changed out
> listener->io_func after the if but before the dispatch.

Hmm, yes, that is a risk I hadn't thought of before.

> Adding a mutex might make QIONetListener slower in the time to grab
> the mutex (even if it is uncontended most of the time), but if we have
> a proper mutex in place, we could at least guarantee that
> listener->io_func, listener->io_data, and listener->io_notify are
> changed as a group, rather than sharded if the polling thread is
> managing a dispatch with listener as its opaque data at the same time
> the user's thread is trying to change the registered callback.

Yeah, a mutex should be ok, as it will be uncontended 99.999% of
the time.

> By itself, GSource DOES have a mutex lock around the GMainContext it
> is attached to; but our particular use of GSource is operating outside
> that locking scheme, so it looks like I've uncovered another latent
> problem.

Agreed.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|