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

[Erik Skultety]

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 at 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.