[libvirt] [PATCH v3] nwfilter: Don't leak @inetaddr
In learnIPAddressThread()the @inetaddr may be leaked. Signed-off-by: ZhiPeng Lu <lu.zhipeng@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); + } } else { if (showError) -- 1.8.3.1
$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@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. ACK and pushed. John
} } else { if (showError)
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@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 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.
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@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
participants (3)
-
Erik Skultety -
John Ferlan -
ZhiPeng Lu