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

[Daniel P. Berrange]

On Mon, May 17, 2010 at 08:44:18AM -0600, Eric Blake wrote:
> 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).

Yes & no. In practice I've never found a need to look at the 'fd'
parameter being logged. So I think it is more important to log
every call, so that we can see cases there 'mon=(null)' instead of
them being skipped.

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

Where is the NULL de-reference ? We're using %p, not %s for logging the
password because we don't want to actually expose the password string
in the logs & %p doesn't de-reference the pointer.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|