[libvirt] [PATCH 2/2] nwfilter: fix error handling

[Peter Krempa]

On 11/30/12 13:51, Peter Krempa wrote:
> On 11/30/12 13:09, Ján Tomko wrote:
>> Commit 4efde75f introduced a return in the cleanup path and removed
>> virNWFilterConfLayerShutdown.
>>
>> Found by coverity:
>> Error: UNREACHABLE (CWE-561):
>>      libvirt-0.10.2/src/nwfilter/nwfilter_driver.c:259: unreachable: This
>>      code cannot be reached: "nwfilterDriverUnlock(driver...".
>> ---
>> I wonder if the if (!privileged) check should be moved before driverState
>> allocation?
>> ---
>>   src/nwfilter/nwfilter_driver.c |   11 +++++------
>>   1 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/nwfilter/nwfilter_driver.c
>> b/src/nwfilter/nwfilter_driver.c
>> index a0ee4f1..cd185e2 100644
>> --- a/src/nwfilter/nwfilter_driver.c
>> +++ b/src/nwfilter/nwfilter_driver.c
>> @@ -174,8 +174,10 @@ nwfilterDriverStartup(bool privileged)
>>       sysbus = virDBusGetSystemBus();
>>   #endif /* HAVE_DBUS */
>>
>> -    if (VIR_ALLOC(driverState) < 0)
>> -        goto alloc_err_exit;
>> +    if (VIR_ALLOC(driverState) < 0) {
>> +        virReportOOMError();
>> +        return -1;
>> +    }
>>
>>       if (virMutexInit(&driverState->lock) < 0)
>>           goto err_free_driverstate;
>> @@ -247,10 +249,7 @@ error:
>>       nwfilterDriverUnlock(driverState);
>>       nwfilterDriverShutdown();
>>
>> -alloc_err_exit:
>> -    return -1;
>
> When you remove this hunk you cause any jump to the error label to fall
> through all of the following labeled error cleanup sections.
>
> Those sections are also called in nwfilterDriverShutdown(). I'm
> wondering if the functions are safe to call even if the driver wasn't
> initialized completely. In that case we might be just calling
> nwfilterDriverShutdown() and get rid of the cleanup labels. If we're not
> allowed to do that, the return statement has to stay there.

virNWFilterDHCPSnoopShutdown(void) doesn't look that it can be called 
without being initialized first, so you will need to either have to fix 
virNWFilterDHCPSnoopShutdown and make sure that other are callable even 
when not initialized, or you need to exit from the function at the point 
the label was before.

>
>> -
>> -    nwfilterDriverUnlock(driverState);
>> +    virNWFilterConfLayerShutdown();
>>
>>   err_techdrivers_shutdown:
>>       virNWFilterTechDriversShutdown();
>>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list