[libvirt PATCH v2 07/10] esx_stream: Use automatic mutex management

[Pavel Hrdina]

On Fri, Mar 04, 2022 at 06:28:37PM +0100, Tim Wiederhake wrote:
> Signed-off-by: Tim Wiederhake <twiederh at redhat.com>
> ---
>  src/esx/esx_stream.c | 65 ++++++++++++++------------------------------
>  1 file changed, 21 insertions(+), 44 deletions(-)
> 
> diff --git a/src/esx/esx_stream.c b/src/esx/esx_stream.c
> index 5b20804bb1..2b49c8dd12 100644
> --- a/src/esx/esx_stream.c
> +++ b/src/esx/esx_stream.c
> @@ -198,9 +198,8 @@ esxStreamTransfer(esxStreamPrivate *priv, bool blocking)
>  static int
>  esxStreamSend(virStreamPtr stream, const char *data, size_t nbytes)
>  {
> -    int result = -1;
>      esxStreamPrivate *priv = stream->privateData;
> -    int status;
> +    VIR_LOCK_GUARD lock = virLockGuardLock(&priv->curl->lock);

Coverity discovered that this change is not correct:

/src/esx/esx_stream.c: 207 in esxStreamSend()
201         esxStreamPrivate *priv = stream->privateData;
202         VIR_LOCK_GUARD lock = virLockGuardLock(&priv->curl->lock);
203
204         if (nbytes == 0)
205             return 0;
206
>>>     CID 389978:  Null pointer dereferences  (REVERSE_INULL)
>>>     Null-checking "priv" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
207         if (!priv) {
208             virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Stream is not open"));
209             return -1;
210         }
211
212         if (priv->mode != ESX_STREAM_MODE_UPLOAD) {

Previously the mutex was initialized after the !priv check, now we will
dereference priv before this check.

>      if (nbytes == 0)
>          return 0;
> @@ -215,38 +214,29 @@ esxStreamSend(virStreamPtr stream, const char *data, size_t nbytes)
>          return -1;
>      }
>  
> -    virMutexLock(&priv->curl->lock);
> -
>      priv->buffer = (char *)data;
>      priv->buffer_size = nbytes;
>      priv->buffer_used = nbytes;
>  
>      if (stream->flags & VIR_STREAM_NONBLOCK) {
>          if (esxStreamTransfer(priv, false) < 0)
> -            goto cleanup;
> +            return -1;
>  
> -        if (priv->buffer_used < priv->buffer_size)
> -            result = priv->buffer_size - priv->buffer_used;
> -        else
> -            result = -2;
> +        if (priv->buffer_used >= priv->buffer_size)
> +            return -2;
>      } else /* blocking */ {
>          do {
> -            status = esxStreamTransfer(priv, true);
> +            int status = esxStreamTransfer(priv, true);
>  
>              if (status < 0)
> -                goto cleanup;
> +                return -1;
>  
>              if (status > 0)
>                  break;
>          } while (priv->buffer_used > 0);
> -
> -        result = priv->buffer_size - priv->buffer_used;
>      }
>  
> - cleanup:
> -    virMutexUnlock(&priv->curl->lock);
> -
> -    return result;
> +    return priv->buffer_size - priv->buffer_used;
>  }
>  
>  static int
> @@ -255,9 +245,8 @@ esxStreamRecvFlags(virStreamPtr stream,
>                     size_t nbytes,
>                     unsigned int flags)
>  {
> -    int result = -1;
>      esxStreamPrivate *priv = stream->privateData;
> -    int status;
> +    VIR_LOCK_GUARD lock = virLockGuardLock(&priv->curl->lock);

And same happens here.

Pavel

>  
>      virCheckFlags(0, -1);
>  
> @@ -274,8 +263,6 @@ esxStreamRecvFlags(virStreamPtr stream,
>          return -1;
>      }
>  
> -    virMutexLock(&priv->curl->lock);
> -
>      priv->buffer = data;
>      priv->buffer_size = nbytes;
>      priv->buffer_used = 0;
> @@ -291,33 +278,25 @@ esxStreamRecvFlags(virStreamPtr stream,
>                  priv->backlog_used - priv->buffer_used);
>  
>          priv->backlog_used -= priv->buffer_used;
> -        result = priv->buffer_used;
>      } else if (stream->flags & VIR_STREAM_NONBLOCK) {
>          if (esxStreamTransfer(priv, false) < 0)
> -            goto cleanup;
> +            return -1;
>  
> -        if (priv->buffer_used > 0)
> -            result = priv->buffer_used;
> -        else
> -            result = -2;
> +        if (priv->buffer_used <= 0)
> +            return -2;
>      } else /* blocking */ {
>          do {
> -            status = esxStreamTransfer(priv, true);
> +            int status = esxStreamTransfer(priv, true);
>  
>              if (status < 0)
> -                goto cleanup;
> +                return -1;
>  
>              if (status > 0)
>                  break;
>          } while (priv->buffer_used < priv->buffer_size);
> -
> -        result = priv->buffer_used;
>      }
>  
> - cleanup:
> -    virMutexUnlock(&priv->curl->lock);
> -
> -    return result;
> +    return priv->buffer_used;
>  }
>  
>  static int
> @@ -348,18 +327,16 @@ esxStreamClose(virStreamPtr stream, bool finish)
>      if (!priv)
>          return 0;
>  
> -    virMutexLock(&priv->curl->lock);
> +    VIR_WITH_MUTEX_LOCK_GUARD(&priv->curl->lock) {
> +        if (finish && priv->backlog_used > 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("Stream has untransferred data left"));
> +            result = -1;
> +        }
>  
> -    if (finish && priv->backlog_used > 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("Stream has untransferred data left"));
> -        result = -1;
> +        stream->privateData = NULL;
>      }
>  
> -    stream->privateData = NULL;
> -
> -    virMutexUnlock(&priv->curl->lock);
> -
>      esxFreeStreamPrivate(&priv);
>  
>      return result;
> -- 
> 2.31.1
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20220317/aaaeed38/attachment.sig>