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

[Jonathon Jongsma]

On Tue, 2016-11-01 at 06:28 -0400, Frediano Ziglio wrote:
> > 
> > 
> > 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.

As I said in an earlier email, this patch does not actually change the
behavior of spice_server_destroy(). It only changes the reds_exit()
function. Qemu can call spice_server_destroy() *right now* and it will
have the same behavior as it will have if this patch is applied. 

And spice_server_destroy() removes the server object from the global
servers list, so it should not get freed at exit if it's freed earlier 
by an explicit call to spice_server_destroy().

So I'm a bit confused by this discussion.

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

OK, thanks for the correction.

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