Re: [libvirt] [PATCH 2/2] nwfilter: Only free inetaddr on virNWFilterIPAddrMapAddIPAddr failure

On Thu, Sep 28, 2017 at 03:26:49PM -0400, John Ferlan wrote: > Flag when virNWFilterIPAddrMapAddIPAddr to allow deletion - keep > @inetaddr around to message after virNWFilterInstantiateFilterLate > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/nwfilter/nwfilter_learnipaddr.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/src/nwfilter/nwfilter_learnipaddr.c > b/src/nwfilter/nwfilter_learnipaddr.c > index 5b95f0e61..567897221 100644 > --- a/src/nwfilter/nwfilter_learnipaddr.c > +++ b/src/nwfilter/nwfilter_learnipaddr.c > @@ -610,6 +610,7 @@ learnIPAddressThread(void *arg) >         sa.data.inet4.sin_family = AF_INET; >         sa.data.inet4.sin_addr.s_addr = vmaddr; >         char *inetaddr; > +        bool failmap = false; The name looks confusing. I'd prefer the name to document either what happened (adding_failed, address_consumed) or what should happen (free_addr). Or rather dropping the bool completely, log the IP address before the AddIpAddr call and remove it from the other VIR_DEBUG message.

> >         /* It is necessary to unlock interface here to avoid > updateMutex and >          * interface ordering deadlocks. Otherwise we are going to > @@ -625,6 +626,7 @@ learnIPAddressThread(void *arg) >             if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) < > 0) { >                 VIR_ERROR(_("Failed to add IP address %s to IP address " >                           "cache for interface %s"), inetaddr, > req->ifname); > +                failmap = true; This assumes that virNWFilterIPAddrMapAddIPAddr consumes inetaddr only if it returns zero. However in a fraction of the unlikely cases when this function call can fail (all of them on OOM), it can consume it even though it returns -1 -- if virNWFilterVarValueCreateSimple succeeds, but virNWFilterHashTablePut fails.

>             } > >             ret = virNWFilterInstantiateFilterLate(req->driver, > @@ -637,7 +639,8 @@ learnIPAddressThread(void *arg) >                                                    req->filterparams); >             VIR_DEBUG("Result from applying firewall rules on " >                       "%s with IP addr %s : %d", req->ifname, > inetaddr, ret); At this point, inetaddr belongs to the hash table protected by the lock that AddIPAddr already released. Can we safely access it here?

Jan > -            VIR_FREE(inetaddr); > +            if (failmap) > +                VIR_FREE(inetaddr); >         } >     } else { >         if (showError) > --  > 2.13.5 > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list