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

[Tim Wiederhake]

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

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