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

[Ján Tomko]

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 at redhat.com>
>---
> 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 at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170929/c8f7d5a2/attachment-0001.sig>