[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] lxcFreezeContainer: avoid test-after-deref of never-NULL pointer



Jim Meyering wrote:

> Eric Blake wrote:
>> On 05/17/2010 11:22 AM, Jim Meyering wrote:
>>> This addresses another coverity-spotted "flaw".
>>> However, since "cgroup" is never NULL after that initial "if" stmt,
>>> the only penalty is that the useless cleanup test would make a reviewer
>>> try to figure out how cgroup could be NULL there.
>>
>> ACK.
>
> Thanks.
>
>>>  cleanup:
>>> -    if (cgroup)
>>> -        virCgroupFree(&cgroup);
>>> +    virCgroupFree(&cgroup);
>>
>> Is this something that the useless-if-before-free test could have
>> caught, rather than waiting for coverity?
>
> Very good idea.
> Here's a patch to do that.
> With this, "make sc_avoid_if_before_free" (part of make syntax-check)
> will now prevent any new useless checks.
> The above was the sole offender.

Actually, this is just the tip of the iceberg.
A couple minutes work have shown that there are many more:

Running this shows there are potentially at least 100
candidate functions:

  aid free|grep '^vi'

Looking at the first few on the list, I've found that most are
indeed free-like:

  N virBufferFreeAndReset
  y virCPUDefFree
  Y virCapabilitiesFree
  y virCapabilitiesFreeGuest
  y virCapabilitiesFreeGuestDomain
  y virCapabilitiesFreeGuestFeature
  y virCapabilitiesFreeGuestMachine
  y virCapabilitiesFreeHostNUMACell
  y virCapabilitiesFreeMachines
  N virCapabilitiesFreeNUMAInfo  (should probably fix)
  y virCgroupFree
  N virConfFree               (diagnoses the "error")
  y virConfFreeList
  y virConfFreeValue

So far, most of the exceptions can be "fixed"
to also be free-like.

Using those few additions uncovered these:

    src/conf/storage_conf.c: if (pool->newDef)
                    virStoragePoolDefFree(pool->newDef)
    src/security/virt-aa-helper.c: if (ctl->caps)
            virCapabilitiesFree(ctl->caps)
    src/util/conf.c: if (cur->value) {
                virConfFreeValue(cur->value);
            }


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]