>
> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel