[libvirt] [PATCH] qemudDomainAttachSCSIDisk: handle empty controller list
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.
From 21ec1a8ae0218a5e8d789410318a973518ffec6c Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 2 Mar 2010 17:45:10 +0100 Subject: [PATCH] qemudDomainAttachSCSIDisk: handle empty controller list
* 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 (qemuAssignDeviceDiskAlias(disk, qemuCmdFlags) < 0) goto error; if (!(devstr = qemuBuildDriveDevStr(disk))) goto error; } 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) goto error; } if (cont->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("SCSI controller %d was missing its PCI address"), cont->idx); -- 1.7.0.1.414.g89213d
According to Jim Meyering on 3/2/2010 9:54 AM:
It's easier/better here just to handle the potential trouble:
* 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).
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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. Thanks, Wolfgang
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 :|
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@redhat.com> 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
On Mon, Mar 15, 2010 at 04:46:31PM +0100, Jim Meyering wrote:
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.
That sounds a good plan to me.
Here's the revert:
From 4d7423fc0e27e86c937dde1a5d62f3b7b6e49451 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> 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)
ACK 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 :|
Jim Meyering wrote:
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.
completely agreed. Thanks, Wolfgang
Here's the revert:
From 4d7423fc0e27e86c937dde1a5d62f3b7b6e49451 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> 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
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Jim Meyering -
Wolfgang Mauerer