[libvirt] [PATCH 0/2] Clean up the nwfilter mess I created
Silly me - touching the nwfilter code... Turns out the reverted patch in 1/2 doesn't cover it as virNWFilterVarValueCreateSimple actually steals @addr, so freeing it on successful return ends up being a very bad thing as I found out with the nwfilter avocado-vt tests. So revert that and just go with the only free on failure logic in patch2 John Ferlan (2): Revert "nwfilter: Fix possible segfault on sometimes consumed variable" nwfilter: Only free inetaddr on virNWFilterIPAddrMapAddIPAddr failure src/conf/nwfilter_ipaddrmap.c | 9 +-------- src/nwfilter/nwfilter_learnipaddr.c | 5 ++++- 2 files changed, 5 insertions(+), 9 deletions(-) -- 2.13.5
This reverts commit 6209bb32e5b6d8c15d55422bb4716b3b31c1c7b2. This turns out to be the wrong adjustment Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_ipaddrmap.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/conf/nwfilter_ipaddrmap.c b/src/conf/nwfilter_ipaddrmap.c index 5668f366d..9c8584ce2 100644 --- a/src/conf/nwfilter_ipaddrmap.c +++ b/src/conf/nwfilter_ipaddrmap.c @@ -26,9 +26,7 @@ #include "internal.h" -#include "viralloc.h" #include "virerror.h" -#include "virstring.h" #include "datatypes.h" #include "nwfilter_params.h" #include "nwfilter_ipaddrmap.h" @@ -54,7 +52,6 @@ virNWFilterIPAddrMapAddIPAddr(const char *ifname, char *addr) { int ret = -1; virNWFilterVarValuePtr val; - char *tmp = NULL; virMutexLock(&ipAddressMapLock); @@ -68,18 +65,14 @@ virNWFilterIPAddrMapAddIPAddr(const char *ifname, char *addr) virNWFilterVarValueFree(val); goto cleanup; } else { - if (VIR_STRDUP(tmp, addr) < 0) + if (virNWFilterVarValueAddValue(val, addr) < 0) goto cleanup; - if (virNWFilterVarValueAddValue(val, tmp) < 0) - goto cleanup; - tmp = NULL; } ret = 0; cleanup: virMutexUnlock(&ipAddressMapLock); - VIR_FREE(tmp); return ret; } -- 2.13.5
On Thu, Sep 28, 2017 at 03:26:48PM -0400, John Ferlan wrote:
This reverts commit 6209bb32e5b6d8c15d55422bb4716b3b31c1c7b2.
This turns out to be the wrong adjustment
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_ipaddrmap.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
ACK Jan
Flag when virNWFilterIPAddrMapAddIPAddr to allow deletion - keep @inetaddr around to message after virNWFilterInstantiateFilterLate Signed-off-by: John Ferlan <jferlan@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; /* 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; } 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); - VIR_FREE(inetaddr); + if (failmap) + VIR_FREE(inetaddr); } } else { if (showError) -- 2.13.5
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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 09/29/2017 11:44 AM, Ján Tomko wrote:
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@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.
to quote Michal from a recent series "Naming is hard" I like the idea of logging the IP before... That resolves some of the oddities.
/* 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.
Ugh... I hate this code.
}
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?
We have been - it's a VIR_DEBUG though... I'll work on a followup. John
Jan
- VIR_FREE(inetaddr); + if (failmap) + VIR_FREE(inetaddr); } } else { if (showError) -- 2.13.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[...]
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.
If virNWFilterHashTablePut fails in virNWFilterIPAddrMapAddIPAddr, then it calls virNWFilterVarValueFree which will VIR_FREE the value that was stored in @val which is returned from virNWFilterVarValueCreateSimple. I still hate the nwfilter code. John
}
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);
[...]
participants (2)
-
John Ferlan -
Ján Tomko