Clang found something that might be a real bug. I suspect that ...drive.controller will always be at least one,

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++) { (...)

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.

From 4d7423fc0e27e86c937dde1a5d62f3b7b6e49451 Mon Sep 17 00:00:00 2001

Daniel P. Berrange wrote: > 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. Thanks. The patch below reverts it. However, there's more to it than that. The controller index, while technically "unsigned", may be derived from an expression like -1 / 7, since virDomainDiskDefAssignAddress does this: void virDomainDiskDefAssignAddress(virDomainDiskDefPtr def) { int idx = virDiskNameToIndex(def->dst); switch (def->bus) { case VIR_DOMAIN_DISK_BUS_SCSI: ... def->info.addr.drive.controller = idx / 7; def->info.addr.drive.bus = 0; def->info.addr.drive.unit = idx % 7; break; case VIR_DOMAIN_DISK_BUS_IDE: ... case VIR_DOMAIN_DISK_BUS_FDC: ... ... and virDiskNameToIndex may return -1. And "idx" is then -1. While we might have gotten lucky with the -1 -> 0 mapping for the .controller member, I doubt a .unit (also "unsigned int") value that's derived from "-1 % 7" (not portable, btw) will be safe. I will propose a patch to change the above function to return an indication of success or failure and update the few callers. They're all in functions that already return success or failure, so this is the only interface that would change.

Here's the revert: From 4d7423fc0e27e86c937dde1a5d62f3b7b6e49451 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com&gt; Date: Mon, 15 Mar 2010 16:43:23 +0100 Subject: [PATCH] Revert f5a6ce44ce8368d4183b69a31b77f67688d9af43 * src/qemu/qemu_driver.c (qemudDomainAttachSCSIDisk): The ".controller" member is an index, and *may* be 0. Reported by Wolfgang Mauerer. --- src/qemu/qemu_driver.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f8ab545..04344a8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6057,12 +6057,6 @@ 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++) { cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i, qemuCmdFlags); if (!cont) -- 1.7.0.2.398.g10c2f