[Spice-devel] [PATCH 1/9] Use spice_server_destroy() at exit

[Frediano Ziglio]

> 
> On Mon, 2016-10-31 at 10:44 -0400, Frediano Ziglio wrote:
> > > 
> > > 
> > > The destructor function was calling reds_cleanup() for each server
> > > at
> > > application exit. But reds_cleanup() only cleans up a couple of
> > > statistics-related variables. We should actually free the servers
> > > by
> > > calling spice_server_destroy(). This left reds_cleanup() unused, so
> > > the
> > > contents of this function were moved inside spice_server_destroy().
> > > ---
> > >  server/reds.c | 30 ++++++++++++------------------
> > >  1 file changed, 12 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/server/reds.c b/server/reds.c
> > > index eeb6010..c03ed18 100644
> > > --- a/server/reds.c
> > > +++ b/server/reds.c
> > > @@ -2904,27 +2904,14 @@ static int reds_init_ssl(RedsState *reds)
> > >      return 0;
> > >  }
> > >  
> > > -static void reds_cleanup(RedsState *reds)
> > > -{
> > > -#ifdef RED_STATISTICS
> > > -    if (reds->stat_shm_name) {
> > > -        shm_unlink(reds->stat_shm_name);
> > > -        free(reds->stat_shm_name);
> > > -        reds->stat_shm_name = NULL;
> > > -    }
> > > -#endif
> > > -}
> > > -
> > >  SPICE_DESTRUCTOR_FUNC(reds_exit)
> > >  {
> > > -    GList *l;
> > > +    GListIter iter;
> > > +    RedsState *reds;
> > >  
> > > -    pthread_mutex_lock(&global_reds_lock);
> > > -    for (l = servers; l != NULL; l = l->next) {
> > > -        RedsState *reds = l->data;
> > > -        reds_cleanup(reds);
> > > +    GLIST_FOREACH(servers, iter, RedsState, reds) {
> > > +        spice_server_destroy(reds);
> > >      }
> > > -    pthread_mutex_unlock(&global_reds_lock);
> > >  }
> > >  
> > >  static inline void on_activating_ticketing(RedsState *reds)
> > > @@ -3695,7 +3682,14 @@ SPICE_GNUC_VISIBLE void
> > > spice_server_destroy(SpiceServer *reds)
> > >      if (reds->main_channel) {
> > >          main_channel_close(reds->main_channel);
> > >      }
> > > -    reds_cleanup(reds);
> > > +
> > > +#ifdef RED_STATISTICS
> > > +    if (reds->stat_shm_name) {
> > > +        shm_unlink(reds->stat_shm_name);
> > > +        free(reds->stat_shm_name);
> > > +        reds->stat_shm_name = NULL;
> > > +    }
> > > +#endif
> > >  
> > >      /* remove the server from the list of servers so that we don't
> > > attempt
> > >      to
> > >       * free it again at exit */
> > 
> > I think the big problem with this patch and following about
> > cleanup is how the API should behave.
> > Usually if there are a XXXXX_new and a XXXXX_destroy IMHO should be
> > up to the user of the library to call the destroy.
> > What happens if tomorrow Qemu decides to call XXXXXX_destroy?
> 
> Qemu can call the destroy function right now regardless of whether this
> patch is applied or not. This patch doesn't really change
> spice_server_destroy() much at all.
> 

This comment is not referred only to this patch.
With these patches we are changing API behavior to destroy everything.
If tomorrow Qemu decide to call spice_server_destroy and leave the sound
cleanup during an atexit you we'll get a double free as the sound pointers
won't be valid.

> > Actually it seems that even Qemu doesn't do much cleanup.
> > However for instance Qemu retains some audio pointers and free them
> > during an atexit callback.
> > If we decide to free everything on reds_exit we need to free
> > everything
> > in the right order, for instance all channel, all clients, RedWorker
> > thread, the dispatcher and so on.
> 
> Sure, but now that we have GObjects that hold references on other
> objects, it shouldn't be all that difficult to get this right.
> 

Yes, but these new patches must go in before the series you sent!

> > Currently we are leaving the RedWorker
> > thread running that potentially will access RedsState already freed.
> 
> How can a RedWorker thread be running after the process exited?
> 

On a multithreaded application exit run in a multithread environment.
Thread are not closed at the beginning of exit(3) process but when
exit_group(2) syscall is called so if you free stuff during atexit
that can be used by another thread (like RedWorker) you'll have a
possible use after free.

> > So surely this patch is a no go for me as we already know that it's
> > racy
> > and leads to crashes depending on the system load.
> 
> How do we know this? I guess I don't quite understand how this patch
> makes anything worse than the current situation.
> 

Try to look at strace of this small program:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <pthread.h>

static void *
thread_proc(void *arg)
{
        sleep(10);
        return NULL;
}

static void
exiting(void)
{
        // use write to avoid accessing FILE* structures
        write(1, "exiting\n", 8);
}

int main(void)
{
        atexit(exiting);

        pthread_t th;
        pthread_create(&th, NULL, thread_proc, NULL);

        sleep(1);

        return 0;
}

> > On how to do it (and back on API behavior) what happens if Qemu wants
> > to
> > keep an interface (think about spice_add_interface) after a possible
> > spice_server_destroy is called (for instance the audio at atexit
> > time)?
> > An implementation could be that users of RedsState structure will
> > retain
> > a reference (that is adding a reference counter to RedsState) so you
> > can
> > safely call spice_server_destroy before destroying any related
> > interface.
> > Note: the destructor functions are called after the atexit ones,
> > unless
> > the library (spice-server in this case) is dynamically unloaded
> > before
> > calling exit (which is not Qemu case).
> > 

Currently there are no ref/unref calls defined in spice public API so
it's fine to define that spice_server_destroy tear down everything but
if we define this behavior adding ref/unref to external APIs in the future
will require extra reference counting, one for internal and another for
external (or something replacing this).

Frediano