[libvirt] [PATCH] util: Prevent libvirtd crash from virPCIDeviceAddressIsEqual()

[Laine Stump]

On 04/28/2013 06:12 AM, Alex Jia wrote:
> GDB backtrace:
>
> Breakpoint 1, virPCIGetVirtualFunctionIndex (pf_sysfs_device_link=0x7fc04400f470 "/sys/bus/pci/devices/0000:03:00.1", vf_sysfs_device_link=<optimized out>, vf_index=vf_index at entry=0x7fc06897b8f4)
>     at util/virpci.c:2107
> 2107            if (virPCIDeviceAddressIsEqual(vf_bdf, virt_fns[i])) {
> (gdb) p *vf_bdf
> $1 = {domain = 0, bus = 3, slot = 16, function = 1}
> (gdb) l
> 2102                             "virtual_functions"), pf_sysfs_device_link);
> 2103            goto out;
> 2104        }
> 2105
> 2106        for (i = 0; i < num_virt_fns; i++) {
> 2107            if (virPCIDeviceAddressIsEqual(vf_bdf, virt_fns[i])) {
> 2108                *vf_index = i;
> 2109                ret = 0;
> 2110                break;
> 2111            }
> (gdb) p num_virt_fns
> $46 = 2
> (gdb) p virt_fns[0]
> $48 = (virPCIDeviceAddressPtr) 0x0
> (gdb) s
> virPCIDeviceAddressIsEqual (bdf2=0x0, bdf1=0x7fc04400f330) at util/virpci.c:1844
> 1844                (bdf1->slot == bdf2->slot) &&
> (gdb) s
>
> Program received signal SIGSEGV, Segmentation fault.
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=957416
>
> Signed-off-by: Alex Jia <ajia at redhat.com>
> ---
>  src/util/virpci.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 97bba74..dda044c 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1897,7 +1897,8 @@ static bool
>  virPCIDeviceAddressIsEqual(virPCIDeviceAddressPtr bdf1,
>                             virPCIDeviceAddressPtr bdf2)
>  {
> -    return ((bdf1->domain == bdf2->domain) &&
> +    return (bdf1 && bdf2 &&
> +            (bdf1->domain == bdf2->domain) &&
>              (bdf1->bus == bdf2->bus) &&
>              (bdf1->slot == bdf2->slot) &&
>              (bdf1->function == bdf2->function));
NACK.

This patch only fixes the symptom (not the root cause), and only in the
case of starting a domain with an <interface type='hostdev'. It doesn't
fix the second crash described in the BZ when running virsh
nodedev-dumpxml - the code path of that doesn't ever get to
virPCIDeviceAddressIsEqual() (but *does* call the function that actually
has the bug).

The root cause of these crashes was a typo introduced just before the
release of 1.0.4. I found that problem and pushed the correct patch on
April 9:

http://libvirt.org/git/?p=libvirt.git;a=commit;h=9579b6bc209b46a0f079b21455b598c817925b48

(Beyond that, I don't like the idea of ignoring a NULL pointer -
virPCIDeviceAddressIsEqual should always be passed non-NULL pointers,
and its only current caller does guarantee that (except for when it has
a bug). If we want virPCIDeviceAddressIsEqual to do something with NULL
pointers, it should be logging an error and failing, but that would
complicate the interface to the function beyond just returning a
true/false (it would have to be tri-state, and the caller would need to
check all three possibilities). I think in this case it's better for the
caller to make sure the pointers it sends are valid.)