[libvirt] [PATCH 3/5] nodedev: Clear the udev_monitor reference once unref'd

[John Ferlan]

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 at redhat.com>
> ---
>  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
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 at redhat.com>

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.

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