[PATCH 05/43] esx: convert virMutex to GMutex

[Rafael Fonseca]

Pino,

thank you for the review. Could you please take a look at patch #42 in
this series? It's the one in which I had to add some explicit unlock
calls, so it'd be good for someone who knows the code to review this
part.

On Tue, Apr 14, 2020 at 8:15 AM Pino Toscano <ptoscano at redhat.com> wrote:
>
> On Friday, 10 April 2020 15:54:32 CEST Rafael Fonseca wrote:
> > @@ -346,11 +342,12 @@ esxStreamClose(virStreamPtr stream, bool finish)
> >  {
> >      int result = 0;
> >      esxStreamPrivate *priv = stream->privateData;
> > +    g_autoptr(GMutexLocker) locker = NULL;
> >
> >      if (!priv)
> >          return 0;
> >
> > -    virMutexLock(&priv->curl->lock);
> > +    locker = g_mutex_locker_new(&priv->curl->lock);
> >
> >      if (finish && priv->backlog_used > 0) {
> >          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > @@ -360,8 +357,6 @@ esxStreamClose(virStreamPtr stream, bool finish)
> >
> >      stream->privateData = NULL;
> >
> > -    virMutexUnlock(&priv->curl->lock);
> > -
> >      esxFreeStreamPrivate(&priv);
>
> Careful here, this is a problematic situation:
> - the lock is indirectly part of the @priv structure
> - esxFreeStreamPrivate calls esxVI_CURL_Free(priv->curl)
> - esxVI_CURL_Free calls virMutexDestroy(&item->lock)
> - lock is still locked, so it will deadlock (or crash, or something
>   not good anyway)
>
> You must unlock the mutex before esxFreeStreamPrivate is called.

Ops, nice catch.

> I did not check other patches of this long series for similar potential
> issues, please do check them.

Ok, will do.

> > diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
> > index 16690edfbe..ed6c6c28cd 100644
> > --- a/src/esx/esx_vi.c
> > +++ b/src/esx/esx_vi.c
> > @@ -433,14 +429,13 @@ int
> >  esxVI_CURL_Upload(esxVI_CURL *curl, const char *url, const char *content)
> >  {
> >      int responseCode = 0;
> > +    g_autoptr(GMutexLocker) locker = g_mutex_locker_new(&curl->lock);
> >
> >      if (!content) {
> >          virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
> >          return -1;
> >      }
> >
> > -    virMutexLock(&curl->lock);
> > -
>
> Careful #2 here about locking earlier: while usually this is not an
> issue, it could be in case the code that was executed without the lock
> held can be called by other code branches with the lock held.
>
> Again, this must be thoroughly checked in the whole patch series.

I'll recheck but I tried to do it in places where it was obvious the
lock was not held because of an early return case.


Att.
-- 
Rafael Fonseca