[libvirt] [PATCH 4/7] qemu: assume 'info chardev' is always available

[Daniel P. Berrange]

On Thu, Nov 05, 2015 at 03:18:12PM -0500, John Ferlan wrote:
> 
> 
> On 11/05/2015 12:33 PM, Daniel P. Berrange wrote:
> > As of QEMU 0.11.0 the 'info chardev' monitor command can be
> > used to report on allocated chardev paths, so we can drop
> > support for parsing QEMU stderr to locate the PTY paths.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> > ---
> >  src/qemu/qemu_process.c | 193 ------------------------------------------------
> >  1 file changed, 193 deletions(-)
> > 
> 
> Ran the series through the Coverity checks...
> 
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 524072c..72c7041 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> 
> [...]
> 
> >  static int
> >  qemuProcessRefreshChannelVirtioState(virQEMUDriverPtr driver,
> > @@ -2140,15 +1956,6 @@ qemuProcessWaitForMonitor(virQEMUDriverPtr driver,
> >          (logfd = qemuDomainOpenLog(driver, vm, pos)) < 0)
> >          return -1;
> >  
> > -    if (logfd != -1 && !virQEMUCapsUsedQMP(qemuCaps)) {
> > -        if (VIR_ALLOC_N(buf, buf_size) < 0)
> > -            goto closelog;
> > -
> > -        if (qemuProcessReadLogOutput(vm, logfd, buf, buf_size,
> > -                                     qemuProcessFindCharDevicePTYs,
> > -                                     "console", 30) < 0)
> > -            goto closelog;
> > -    }
> >  
> >      VIR_DEBUG("Connect monitor to %p '%s'", vm, vm->def->name);
> >      if (qemuConnectMonitor(driver, vm, asyncJob, logfd) < 0)
> > 
> 
> Complaint from Coverity is in the cleanup: section
> 
> 
> (11) Event cond_false: 	Condition "virQEMUCapsUsedQMP(qemuCaps)", taking
> false branch
> 
> 1993 	        if (virQEMUCapsUsedQMP(qemuCaps)) {
> 1994 	            if (VIR_ALLOC_N(buf, buf_size) < 0)
> 1995 	                goto closelog;
> 
> (12) Event if_end: 	End of if statement
> 
> 1996 	        }
> 1997 	
> 
> (13) Event var_deref_model: 	Passing null pointer "buf" to "strlen",
> which dereferences it.
> Also see events: 	[assign_zero]
> 
> 
> So if virQEMUCapsUsedQMP fails, then buf is NULL

Ok, we need to unconditionally allocate buf, and get ridof the strlen
call. So I'm adding this patch

@@ -1987,20 +1987,16 @@ qemuProcessWaitForMonitor(virQEMUDriverPtr driver,
     virHashFree(info);
 
     if (pos != -1 && kill(vm->pid, 0) == -1 && errno == ESRCH) {
-        int len;
         /* VM is dead, any other error raised in the interim is probably
          * not as important as the qemu cmdline output */
-        if (virQEMUCapsUsedQMP(qemuCaps)) {
-            if (VIR_ALLOC_N(buf, buf_size) < 0)
-                goto closelog;
-        }
+        if (VIR_ALLOC_N(buf, buf_size) < 0)
+            goto closelog;
 
-        len = strlen(buf);
         /* best effort seek - we need to reset to the original position, so that
          * a possible read of the fd in the monitor code doesn't influence this
          * error delivery option */
         ignore_value(lseek(logfd, pos, SEEK_SET));
-        qemuProcessReadLog(logfd, buf + len, buf_size - len - 1, 0, true);
+        qemuProcessReadLog(logfd, buf, buf_size - 1, 0, true);
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("process exited while connecting to monitor: %s"),
                        buf);


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|