[libvirt] [PATCH v3] nwfilter: Don't leak @inetaddr

[John Ferlan]

On 09/27/2017 09:34 AM, Erik Skultety wrote:
> On Wed, Sep 27, 2017 at 08:06:42AM -0400, John Ferlan wrote:
>>
>> $subj:
>>
>> nwfilter: Fix memory leak in learnIPAddressThread
>>
>> On 09/26/2017 09:01 PM, ZhiPeng Lu wrote:
>>> In learnIPAddressThread()the @inetaddr may be leaked.
>>>
>>
>> Changing this to:
>>
>> Don't leak @inetaddr within the done: processing when attempting
>> to instantiate the filter.
>>
>>
>>> Signed-off-by: ZhiPeng Lu <lu.zhipeng at zte.com.cn>
>>> ---
>>>  src/nwfilter/nwfilter_learnipaddr.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
>>> index cfd92d9..0cadf73 100644
>>> --- a/src/nwfilter/nwfilter_learnipaddr.c
>>> +++ b/src/nwfilter/nwfilter_learnipaddr.c
>>> @@ -605,6 +605,7 @@ learnIPAddressThread(void *arg)
>>>
>>>      if (req->status == 0) {
>>>          int ret;
>>> +        int mapipret = -1;
>>>          virSocketAddr sa;
>>>          sa.len = sizeof(sa.data.inet4);
>>>          sa.data.inet4.sin_family = AF_INET;
>>> @@ -622,7 +623,7 @@ learnIPAddressThread(void *arg)
>>>          virNWFilterUnlockIface(req->ifname);
>>>
>>>          if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) {
>>> -            if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) < 0) {
>>> +            if ((mapipret = virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr)) < 0) {
>>>                  VIR_ERROR(_("Failed to add IP address %s to IP address "
>>>                            "cache for interface %s"), inetaddr, req->ifname);
>>>              }
>>> @@ -637,6 +638,9 @@ learnIPAddressThread(void *arg)
>>>                                                     req->filterparams);
>>>              VIR_DEBUG("Result from applying firewall rules on "
>>>                        "%s with IP addr %s : %d", req->ifname, inetaddr, ret);
>>> +            if (mapipret < 0)
>>> +                VIR_FREE(inetaddr);
>>> +
>>
>> What's the purpose of mapipret?  @inetaddr isn't allocated because of
>> the return of virNWFilterIPAddrMapAddIPAddr it's allocated because of
>> virSocketAddrFormat and thus would need to be VIR_FREE'd regardless.
>>
>> I've fixed that by just removing the unnecessary @mapipret and just
>> using VIR_FREE() at the end of the if statement.
> 
> That's a wrong fix, because as ZhiPeng Lu noted in previous version, you freed
> something that is assigned to the ipAddressMap pool on success, so the next
> time you access it, you'll get a SIGSEGV
> 

Ugh! My brain couldn't parse what he wrote. I'm glad you could.

Still this pile of code is awful....

virNWFilterIPAddrMapAddIPAddr takes @addr and if not found in
ipAddressMap->hashTable, it creates a @val from @addr and inserts @val
into a hash table and the caller can/should free @addr on success.

If it is found, then virNWFilterVarValueAddValue is called and on
success, yes, it consumes @addr and caller shouldn't free @addr

So how is the caller to know that @addr has been consumed or not on
success. For one instance it is, but on the other it isn't.  Certainly
on failure it's not consumed and free-able.

> Erik
> 
> Btw I wanted to respond to the previous version in the morning, but I couldn't
> come up with anything else than either using STRDUP in
> virNWFilterIPAddrMapAddIPAddr, so then we could go with the proposal you just
> pushed, or we could refactor the whole nwfilter as it's a real mess...(that's a
> joke btw, I don't think anyone is volunteering to that any time soon) - it's a
> shame that the nwfilter code is kinda "no touchy" unless something crashes.
> 

So I think the fix to this awful code would be to use VIR_STRDUP() in
virNWFilterIPAddrMapAddIPAddr for the else condition into a @tmp
variable that would be used in call to virNWFilterVarValueAddValue. I'll
send something shortly.


John