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
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.
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
