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