1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v3] nwfilter: Don't leak @inetaddr

ZhiPeng Lu posted 1 patch 7 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1506474099-5636-1-git-send-email-lu.zhipeng@zte.com.cn
src/nwfilter/nwfilter_learnipaddr.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
[libvirt] [PATCH v3] nwfilter: Don't leak @inetaddr
Posted by ZhiPeng Lu 7 years, 7 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] nwfilter: Don't leak @inetaddr
Posted by John Ferlan 7 years, 7 months ago 
$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)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] nwfilter: Don't leak @inetaddr
Posted by Erik Skultety 7 years, 7 months ago 
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.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] nwfilter: Don't leak @inetaddr
Posted by John Ferlan 7 years, 7 months ago 

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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.