[v2] Automatic mutex management - part 3

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

Posted by Tim Wiederhake 3 years, 11 months ago

Signed-off-by: Tim Wiederhake <twiederh@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);
 
     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);
 
     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

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

Posted by Pavel Hrdina 3 years, 10 months ago

On Fri, Mar 04, 2022 at 06:28:37PM +0100, Tim Wiederhake wrote:
> Signed-off-by: Tim Wiederhake <twiederh@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
>

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

Posted by Tim Wiederhake 3 years, 10 months ago

On Thu, 2022-03-17 at 10:44 +0100, Pavel Hrdina wrote:
> On Fri, Mar 04, 2022 at 06:28:37PM +0100, Tim Wiederhake wrote:
> > Signed-off-by: Tim Wiederhake <twiederh@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

Thanks for making me aware of that mistake. I will set out to fix this
immediately.

Tim

> >  
> >      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
> >