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

[Wolfgang Mauerer]

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