Re: [libvirt] [PATCH v3] nwfilter: Don't leak @inetaddr
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(a)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