[libvirt] [PATCH] qemudDomainAttachSCSIDisk: handle empty controller list

[Daniel P. Berrange]

On Mon, Mar 15, 2010 at 03:56:55PM +0100, Wolfgang Mauerer wrote:
> Jim Meyering wrote:
> > Clang found something that might be a real bug.
> > I suspect that ...drive.controller will always be at least one,
> it can - explanation below.
> 
> > but we should not have to dive into the code trying to figure
> > that out.  It's easier/better here just to handle the potential trouble:
> > 
> > clang saw that if it *was* zero, then the following "for" loop
> > would not be entered, and "cont" would not be initialized.
> > On the very next statement "cont" (uninitialized) would be dereferenced.
> (...)
> > * src/qemu/qemu_driver.c (qemudDomainAttachSCSIDisk): Handle
> > the (theoretical) case of an empty controller list, so that
> > clang does not think the subsequent dereference of "cont"
> > would dereference an undefined variable (due to preceding
> > loop not iterating even once).
> > ---
> >  src/qemu/qemu_driver.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 7f7c459..efb1857 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -5671,18 +5671,24 @@ static int qemudDomainAttachSCSIDisk(struct qemud_driver *driver,
> (...)
> >      if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags)))
> >          goto error;
> > 
> > +    if (disk->info.addr.drive.controller <= 0) {
> > +        qemuReportError(VIR_ERR_INTERNAL_ERROR,
> > +                        _("no drive controller for %s"), disk->dst);
> > +        goto error;
> > +    }
> > +
> >      for (i = 0 ; i <= disk->info.addr.drive.controller ; i++) {
> > (...)
> 
> disk->info.addr.drive.controller does not denote the number of
> available controllers, but an index -- which can very well be zero,
> and the loop is always entered. Besides, checking for < 0 in
> the test does not make sense since
> _virDomainDeviceDriveAddress.controller is unsigned.
> 
> Since this commit breaks SCSI disk hotplug on controller 0,
> please revert it.

Agreed, this is definitely broken. 

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