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

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

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