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

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