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

[Alex Jia]

On 04/29/2013 02:48 AM, Laine Stump wrote:
> 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

Yes, I just noticed this, it should be different code path.

> virPCIDeviceAddressIsEqual() (but *does* call the function that actually
> has the bug).

Yes, another 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

It should be okay if the patch is backported into rhel.
> (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

Yes, the function interface will be more complex and return value should
be tri-state not simple true/false.

> check all three possibilities). I think in this case it's better for the
> caller to make sure the pointers it sends are valid.)

Yes, a simple way is the caller makes sure pointers is Non-NULL, maybe,
we need also to add comment "The caller should make sure the pointers
it sends are valid" into the virPCIDeviceAddressIsEqual(). However,
in order to avoid the caller missing argument judgement, I think we
should also fix virPCIDeviceAddressIsEqual().

Thanks for your comments,
Alex
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list