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