Re: [libvirt] [PATCH] qemu: Alter condition to avoid possible NULL deref

[Michal Privoznik]

On 02/09/2018 04:33 PM, John Ferlan wrote:
> Commit 'f0f2a5ec2' neglected to adjust the if condition to split
> out the possibility that the @watchdog is NULL when altering the
> message to add detail about the model.
> 
> Just split out the condition and use previous/original message, but
> with the new message code.
> 
> Found by Coverity
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_hotplug.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index c7bf25eee..3291ce613 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -5159,11 +5159,16 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
>  virDomainWatchdogDefPtr watchdog = vm->def->watchdog;
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  
> +if (!watchdog) {
> +virReportError(VIR_ERR_DEVICE_MISSING, "%s",
> +   _("watchdog device not present in domain 
> configuration"));
> +return -1;
> +}
> +
>  /* While domains can have up to one watchdog, the one supplied by the 
> user
>   * doesn't necessarily match the one domain has. Refuse to detach in such
>   * case. */
> -if (!(watchdog &&
> -  watchdog->model == dev->model &&
> +if (!(watchdog->model == dev->model &&
>watchdog->action == dev->action &&
>virDomainDeviceInfoAddressIsEqual(>info, >info))) {
>  virReportError(VIR_ERR_DEVICE_MISSING,
> 

Oh I see now. The problem is not with the condition, but this
virReportError() uses watchdog->model (for better message) which is
dangerous in case @watchdog == NULL. I though that there's something bad
with the condition itself.

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Alter condition to avoid possible NULL deref

[Chen Hanxiao]

At 2018-02-09 23:33:38, "John Ferlan"  wrote:
>Commit 'f0f2a5ec2' neglected to adjust the if condition to split
>out the possibility that the @watchdog is NULL when altering the
>message to add detail about the model.
>
>Just split out the condition and use previous/original message, but
>with the new message code.
>
>Found by Coverity
>
>Signed-off-by: John Ferlan 
>---
> src/qemu/qemu_hotplug.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>index c7bf25eee..3291ce613 100644
>--- a/src/qemu/qemu_hotplug.c
>+++ b/src/qemu/qemu_hotplug.c
>@@ -5159,11 +5159,16 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
> virDomainWatchdogDefPtr watchdog = vm->def->watchdog;
> qemuDomainObjPrivatePtr priv = vm->privateData;
> 
>+if (!watchdog) {
>+virReportError(VIR_ERR_DEVICE_MISSING, "%s",
>+   _("watchdog device not present in domain 
>configuration"));
>+return -1;
>+}
>+
> /* While domains can have up to one watchdog, the one supplied by the user
>  * doesn't necessarily match the one domain has. Refuse to detach in such
>  * case. */
>-if (!(watchdog &&
>-  watchdog->model == dev->model &&
>+if (!(watchdog->model == dev->model &&
>   watchdog->action == dev->action &&
>   virDomainDeviceInfoAddressIsEqual(>info, >info))) {
> virReportError(VIR_ERR_DEVICE_MISSING,
>-- 

Reviewed-by: Chen Hanxiao 

Regards,
- Chen

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list