Libguestfs - Jul 2019 - Re: [libnbd PATCH] docs: Mention that nbd_close is not thread-safe

Closing the handle in one thread while another thread is locked causes
undefined behavior (as closing does not obtain a lock, and can cause
use-after-free or other nasty problems to the thread that does own the
lock).  However, it is not sensible to try and obtain a lock in
nbd_close, as POSIX says that it is also undefined for any other
thread to wait on a mutex that has already been destroyed.  Therefore,
we don't need to change our code, but merely remind users that
nbd_close is not safe until all other uses of the handle have ceased.
---

 generator/generator | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/generator/generator b/generator/generator
index 896ad2a..bdd8fd7 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3532,7 +3532,8 @@ for how to get further details of the error.
 Closes the handle and frees any associated resources.  The final
 status of any command that has not been retired (whether by
 C<nbd_aio_command_completed> or by a low-level completion callback
-returning C<1>) is lost.
+returning C<1>) is lost.  This function is not safe to call while
+any other thread is still using any C<nbd_*> API on the same handle.

 =head1 GETTING THE LATEST ERROR MESSAGE IN THE THREAD

-- 
2.20.1

On Wed, Jul 24, 2019 at 03:14:28PM -0500, Eric Blake
wrote:
> Closing the handle in one thread while another thread is locked causes
> undefined behavior (as closing does not obtain a lock, and can cause
> use-after-free or other nasty problems to the thread that does own the
> lock).  However, it is not sensible to try and obtain a lock in
> nbd_close, as POSIX says that it is also undefined for any other
> thread to wait on a mutex that has already been destroyed.  Therefore,
> we don't need to change our code, but merely remind users that
> nbd_close is not safe until all other uses of the handle have ceased.
> ---
> 
>  generator/generator | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/generator/generator b/generator/generator
> index 896ad2a..bdd8fd7 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -3532,7 +3532,8 @@ for how to get further details of the error.
>  Closes the handle and frees any associated resources.  The final
>  status of any command that has not been retired (whether by
>  C<nbd_aio_command_completed> or by a low-level completion callback
> -returning C<1>) is lost.
> +returning C<1>) is lost.  This function is not safe to call while
> +any other thread is still using any C<nbd_*> API on the same handle.
> 
>  =head1 GETTING THE LATEST ERROR MESSAGE IN THE THREAD
ACK.

Yes it's not safe to call nbd_close until all other uses of
the same handle from any other thread are over.

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/

On Thu, Jul 25, 2019 at 09:41:01AM +0100, Richard W.M. Jones
wrote:
>On Wed, Jul 24, 2019 at 03:14:28PM -0500, Eric Blake wrote:
>> Closing the handle in one thread while another thread is locked causes
>> undefined behavior (as closing does not obtain a lock, and can cause
>> use-after-free or other nasty problems to the thread that does own the
>> lock).  However, it is not sensible to try and obtain a lock in
>> nbd_close, as POSIX says that it is also undefined for any other
>> thread to wait on a mutex that has already been destroyed.  Therefore,
>> we don't need to change our code, but merely remind users that
>> nbd_close is not safe until all other uses of the handle have ceased.
>> ---
>>
>>  generator/generator | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/generator/generator b/generator/generator
>> index 896ad2a..bdd8fd7 100755
>> --- a/generator/generator
>> +++ b/generator/generator
>> @@ -3532,7 +3532,8 @@ for how to get further details of the error.
>>  Closes the handle and frees any associated resources.  The final
>>  status of any command that has not been retired (whether by
>>  C<nbd_aio_command_completed> or by a low-level completion
callback
>> -returning C<1>) is lost.
>> +returning C<1>) is lost.  This function is not safe to call
while
>> +any other thread is still using any C<nbd_*> API on the same
handle.
>>
>>  =head1 GETTING THE LATEST ERROR MESSAGE IN THE THREAD
>
>ACK.
>
>Yes it's not safe to call nbd_close until all other uses of
>the same handle from any other thread are over.
>
Would it be too much of a trouble to add reference counting and give users a way
to "copy" of the handle?  It wouldn't be a copy, but merely an
increment on the
reference counter.  Or is it not worth doing that?
>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/
>
>_______________________________________________
>Libguestfs mailing list
>Libguestfs@redhat.com
>https://www.redhat.com/mailman/listinfo/libguestfs