On Tue, Aug 01, 2017 at 03:25:37PM -0400, John Ferlan wrote:
>
>
> On 07/26/2017 02:44 AM, Erik Skultety wrote:
> > Since we only have one udev_monitor reference throughout libvirt, we
> > should either clear the pointer we copy around to prevent invalid memory
> > access once we unref this single reference, or start using reference
> > counting provided by libudev. This patch follows the former and only
> > clears the pointer.
> >
> > Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx>
> > ---
> > src/node_device/node_device_udev.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
>
> What's here, works, but expanding things out a bit for this one...
>
> Since we have the nodeDeviceLock, why not within:
>
> if (priv) {
> }
>
> do a VIR_FREE(driver->privateData); inside the if, that should set
> driver->privateData = NULL too. Avoids something finding driver->priv
> later in the instant that the nodeDeviceUnlock is run and something else
Good point.
> gets the lock and looks @priv... IOW: See, patch 5 comments.
>
> I think if driver->priv is set to NULL, then there'd be no need to set
> udev_monitor to NULL, but it cannot hurt especially if this is some sort
> of race with udevEventHandleCallback, so...
>
> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
>
> But I do think the VIR_FREE(priv) should move and be a
> VIR_FREE(driver->privateData)... Note, not @priv because that won't set
> @privateData = NULl which is what we'd want. You may even want to leave
> a comment about that indicating trying to close a race w/ event handling.
I don't know, to me commenting such a change would appear as a bogus comment,
IOW, you always have to protect access to shared data (unless implementing a
lockless algorithm) so I think this change is self-explanatory, but anyways
thanks for pointing it out, I'll definitely adjust the code.
Erik
>
> John
>
> > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> > index ea10dc3ce..cd19e79c1 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1581,6 +1581,7 @@ nodeStateCleanup(void)
> > if (udev_monitor != NULL) {
> > udev = udev_monitor_get_udev(udev_monitor);
> > udev_monitor_unref(udev_monitor);
> > + priv->udev_monitor = NULL;
> > }
> > }
> >
> >
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
