[libvirt] [PATCH v2 0/2] Clean up the nwfilter mess I created
v1: https://www.redhat.com/archives/libvir-list/2017-September/msg01072.html Changes: * Patch1: No change, ACK'd, but not safe to push yet either.. * Patch2: Rather than have virNWFilterIPAddrMapAddIPAddr consume the input @addr, let's make a copy of the input parameter and manage it within that code so that it wouldn't be consumed on virNWFilterHashTablePut failure after virNWFilterVarValueCreateSimple success. John Ferlan (2): Revert "nwfilter: Fix possible segfault on sometimes consumed variable" nwfilter: Don't have virNWFilterIPAddrMapAddIPAddr consume input src/conf/nwfilter_ipaddrmap.c | 16 +++++++++------- src/nwfilter/nwfilter_dhcpsnoop.c | 3 --- 2 files changed, 9 insertions(+), 10 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 pure success paths, virNWFilterIPAddrMapAddIPAddr was validly consuming the input @addr; however, on failure paths it was possible that virNWFilterVarValueCreateSimple succeed, but virNWFilterHashTablePut failed resulting in virNWFilterVarValueFree being called to clean up @val which also cleaned up the input @addr. Thus the caller had no way to determine on failure whether it too should clean up the passed parameter. Instead, let's create a copy of the input @addr, then handle that properly in the API allowing/forcing the caller to free it's own copy of the input parameter. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_ipaddrmap.c | 13 +++++++++++-- src/nwfilter/nwfilter_dhcpsnoop.c | 3 --- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/conf/nwfilter_ipaddrmap.c b/src/conf/nwfilter_ipaddrmap.c index 9c8584ce2..54e6d0f0f 100644 --- a/src/conf/nwfilter_ipaddrmap.c +++ b/src/conf/nwfilter_ipaddrmap.c @@ -26,7 +26,9 @@ #include "internal.h" +#include "viralloc.h" #include "virerror.h" +#include "virstring.h" #include "datatypes.h" #include "nwfilter_params.h" #include "nwfilter_ipaddrmap.h" @@ -51,28 +53,35 @@ int virNWFilterIPAddrMapAddIPAddr(const char *ifname, char *addr) { int ret = -1; + char *addrCopy; virNWFilterVarValuePtr val; + if (VIR_STRDUP(addrCopy, addr) < 0) + return -1; + virMutexLock(&ipAddressMapLock); val = virHashLookup(ipAddressMap->hashTable, ifname); if (!val) { - val = virNWFilterVarValueCreateSimple(addr); + val = virNWFilterVarValueCreateSimple(addrCopy); if (!val) goto cleanup; + addrCopy = NULL; ret = virNWFilterHashTablePut(ipAddressMap, ifname, val); if (ret < 0) virNWFilterVarValueFree(val); goto cleanup; } else { - if (virNWFilterVarValueAddValue(val, addr) < 0) + if (virNWFilterVarValueAddValue(val, addrCopy) < 0) goto cleanup; + addrCopy = NULL; } ret = 0; cleanup: virMutexUnlock(&ipAddressMapLock); + VIR_FREE(addrCopy); return ret; } diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 4436e396f..743136277 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -476,9 +476,6 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl, if (virNWFilterIPAddrMapAddIPAddr(req->ifname, ipaddr) < 0) goto exit_snooprequnlock; - /* ipaddr now belongs to the map */ - ipaddr = NULL; - if (!instantiate) { rc = 0; goto exit_snooprequnlock; -- 2.13.5
On Fri, Sep 29, 2017 at 04:02:48PM -0400, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2017-September/msg01072.html
Changes:
* Patch1: No change, ACK'd, but not safe to push yet either..
* Patch2: Rather than have virNWFilterIPAddrMapAddIPAddr consume the input @addr, let's make a copy of the input parameter and manage it within that code so that it wouldn't be consumed on virNWFilterHashTablePut failure after virNWFilterVarValueCreateSimple success.
John Ferlan (2): Revert "nwfilter: Fix possible segfault on sometimes consumed variable" nwfilter: Don't have virNWFilterIPAddrMapAddIPAddr consume input
src/conf/nwfilter_ipaddrmap.c | 16 +++++++++------- src/nwfilter/nwfilter_dhcpsnoop.c | 3 --- 2 files changed, 9 insertions(+), 10 deletions(-)
ACK series Jan
participants (2)
-
John Ferlan -
Ján Tomko