[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] unlock eventLoop before calling callback function



On Mon, Mar 07, 2011 at 02:13:23PM +0100, Jiri Denemark wrote:
> On Mon, Mar 07, 2011 at 11:15:38 +0000, Daniel P. Berrange wrote:
> > > -        if (eventLoop.handles[i].ff)
> > > +        if (eventLoop.handles[i].ff) {
> > > +            virMutexUnlock(&eventLoop.lock);
> > >              (eventLoop.handles[i].ff)(eventLoop.handles[i].opaque);
> > > +            virMutexLock(&eventLoop.lock);
> > > +        }
> >
> > I'm a little concerned as to whether the rest of the code in
> > virEventCleanupHandles/CleanupTimeouts is safe, if we release
> > the lock here. eg, if some other thread calls virEventAddTimeout
> > of AddHandle, is there any way this could cause problems for us
> > here. So far I think this is safe because AddTimeout/AddHandle
> > will simply append to the end of the array we're iterating over,
> > but would like a second opinion before ACK'ing
> 
> I don't think it's safe to unlock eventloop.lock even in the existing
> Dispatch{Timeouts,Handles} cases because Add{Timeout,Handle} use realloc which
> is allowed to allocate a new array, move the contents of the old one into it
> and free the old array. So the for loop can easily end up accessing memory
> which has already been freed.

That's a very unlikely scenario, but yes it could happen. We'd need to
save a copy of the row we're accessing. Thus instead of

      if (eventLoop.handles[i].ff) {
          virMutexUnlock(&eventLoop.lock);
          (eventLoop.handles[i].ff)(eventLoop.handles[i].opaque);
          virMutexLock(&eventLoop.lock);
      }

We probably need

      if (eventLoop.handles[i].ff) {
          virFreeCallback ff = eventLoop.handles[i].ff;
          void *opaque = eventLoop.handles[i].opaque;
          virMutexUnlock(&eventLoop.lock);
          ff(opaque);
          virMutexLock(&eventLoop.lock);
      }


Regards,
Daniel
--
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]