[Libguestfs] [PATCH v3 3/5] threads: Use thread-local storage for errors.

[Richard W.M. Jones]

On Mon, Jul 10, 2017 at 01:48:13PM +0200, Pino Toscano wrote:
> On Tuesday, 27 June 2017 13:55:57 CEST Richard W.M. Jones wrote:
> > We permit the following constructs in libguestfs code:
> > 
> >   if (guestfs_some_call (g) == -1) {
> >     fprintf (stderr, "failed: error is %s\n", guestfs_last_error (g));
> >   }
> > 
> > and:
> > 
> >   guestfs_push_error_handler (g, NULL, NULL);
> >   guestfs_some_call (g);
> >   guestfs_pop_error_handler (g);
> > 
> > Neither of these would be safe if we allowed the handle to be used
> > from threads concurrently, since the error string or error handler
> > could be changed by another thread.
> > 
> > Solve this in approximately the same way that libvirt does: by making
> > the error, current error handler, and stack of error handlers use
> > thread-local storage (TLS).
> > 
> > The implementation is not entirely straightforward, mainly because
> > POSIX doesn't give us useful destructor behaviour, so effectively we
> > end up creating our own destructor using a linked list.
> 
> I'm not sure which behaviour you are referring to, but it should work
> just fine -- in the destructor function, cast the void* argument to the
> error_data struct, and free the linked list associated.

I believe the problem is this scenario:

(1) Thread A creates a handle and calls some function which
initializes the g->error_data field (eg. any API function that returns
an error would do that).

(2) Later, thread B closes the same handle.

In step (1), pthread_key_create is called, and let's imagine that it
sets a destructor to clean up g->error_data.  The thread A version of
g->error_data points to some structure, but the thread B version of
g->error_data is NULL.

In step (2), pthread_key_delete is called by guestfs_close to clean up
the TLS key.  However it never calls the destructor (see the
documentation of pthread_key_delete), and so it never frees thread A's
g->error_data.  Furthermore we cannot manually free it because thread B
has no way to see thread A's TLS data.

We could arrange so that guestfs_close does NOT call
pthread_key_delete, but then I believe there is a possibility that the
same key could be reused.  Since it's effectively just &g->error_data
(the address of a pthread_key_t object), if a new guestfs_h struct is
allocated at the same address then the same key is reused.  This is
not permitted (see pthread_key_create manual "Non-Idempotent Data Key
Creation").

Note that it's really fine to pass handles around and close them in
different threads, eg. if we have a dispatch thread which hands off
handles to worker threads.  Having to pass the handles back to the
dispatch thread to close them would be considerable extra work and
really be API-breaking.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/