Re: [Libguestfs] [libnbd PATCH v2] lib/errors.c: Fix assert fail in exit path in multi-threaded code

When a highly multi-threaded program such as nbdcopy encounters an error, there is a race condition in the library which can cause an assertion failure and thus a core dump: (1) An error occurs on one of the threads. nbdcopy calls exit(3). (2) In lib/errors.c, the destructor calls pthread_key_delete. (3) Another thread which is still running also encounters an error, and inside libnbd the library calls set_error(). (4) The call to set_error() calls pthread_getspecific. This is undefined behavior per POSIX, since the key has already been destroyed in step (2), but glibc handles it by returning NULL with an EINVAL error (POSIX recommends, but can't mandate, that course of action for technical reasons). In any case, the error message is lost, and any subsequent call to nbd_get_error() will return NULL. (5) We enter the %DEAD state, where there is an assertion that nbd_get_error() is not NULL. This assertion is supposed to be for checking that the code called set_error() before entering the %DEAD state. Although we did call set_error(), the error message was lost at step (4) and nbd_get_error() will always return NULL. This assertion failure causes a crash. There aren't any good ways to fix this. We have proven that the assertions are too tight (it IS possible for one thread's first use of libnbd API, which causes the allocation of a thread-local error context, to occur after another thread has already triggered the destructor via exit(3)), so remove those. Meanwhile, since POSIX says any use of a destroyed key is undefined, add some atomic bookkeeping such that we track a reference count of how many threads have allocated an error context and subsequently had the per-thread data destructor called. Only call pthread_key_delete() if all threads had the chance to exit; leaking small amounts of memory is preferable to undefined behavior. Affected tests that now produce the new leak message: - copy/copy-nbd-error.sh (known issue - we probably want to improve nbdcopy to gracefully issue NBD_CMD_DISC on error, rather than outright calling exit(), to work around the assertion failure in nbdkit 1.32.5) - various fuse/* (not sure if we can address that; cleaning up FUSE is tricky) This reverts a small part of commit 831cbf7ee14c ("generator: Allow DEAD state actions to run"). Thanks: Richard W.M. Jones <rjones(a)redhat.com&gt; Signed-off-by: Eric Blake <eblake(a)redhat.com&gt;

@@ -87,16 +92,23 @@ free_errors_key (void *vp) static struct last_error * allocate_last_error_on_demand (void) { - struct last_error *last_error = pthread_getspecific (errors_key); + struct last_error *last_error = NULL; - if (!last_error) { - last_error = calloc (1, sizeof *last_error); - if (last_error) { - int err = pthread_setspecific (errors_key, last_error); - if (err != 0) { - /* This is not supposed to happen (XXX). */ - fprintf (stderr, "%s: %s: %s\n", "libnbd", "pthread_setspecific", - strerror (err)); + if (key_use_count) { + last_error = pthread_getspecific (errors_key); + if (!last_error) { + last_error = calloc (1, sizeof *last_error); + if (last_error) { + int err = pthread_setspecific (errors_key, last_error); + key_use_count++; + if (err != 0) { + /* This is not supposed to happen (XXX). */ + fprintf (stderr, "%s: %s: %s\n", "libnbd", "pthread_setspecific", + strerror (err)); + free (last_error); + last_error = NULL; + key_use_count--; + } } } }