[libvirt] [PATCH 2/2] Protect against NULL pointer flaws in monitor usage

[Eric Blake]

On 05/17/2010 05:53 AM, Daniel P. Berrange wrote:
> History has shown that there are frequent bugs in the QEMU driver
> code leading to the monitor being invoked with a NULL pointer.
> Although the QEMU driver code should always report an error in
> this case before invoking the monitor, as a safety net put in a
> generic check in the monitor code entry points.
> 
> * src/qemu/qemu_monitor.c: Safety net to check for NULL monitor
>   object
> ---
>  src/qemu/qemu_monitor.c |  409 +++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 346 insertions(+), 63 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 2ce3d56..ec22c20 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -894,7 +894,13 @@ int qemuMonitorEmitGraphics(qemuMonitorPtr mon,
>  int qemuMonitorSetCapabilities(qemuMonitorPtr mon)
>  {
>      int ret;
> -    DEBUG("mon=%p, fd=%d", mon, mon->fd);
> +    DEBUG("mon=%p", mon);
> +
> +    if (!mon) {
> +        qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> +                        _("monitor must not be NULL"));
> +        return -1;
> +    }

Wouldn't it be better to move the DEBUG() to be after the (!mon) check,
so that we can still print mon->fd?  (Throughout the patch).

> @@ -1017,7 +1065,14 @@ int qemuMonitorSetVNCPassword(qemuMonitorPtr mon,
>                                const char *password)
>  {
>      int ret;
> -    DEBUG("mon=%p, fd=%d", mon, mon->fd);
> +    DEBUG("mon=%p, password=%p",
> +          mon, password);
> +
> +    if (!mon) {
> +        qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> +                        _("monitor must not be NULL"));
> +        return -1;
> +    }

Likewise.  And while it was nice to add password=%p,...

>  
>      if (!password)
>          password = "";

...you may have just dereferenced another NULL pointer (at least DEBUG
tends to only be used with glibc, where you get a sane "(null)" instead
of a crash).

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100517/f7fb01a7/attachment-0001.sig>