ngx_resolver.c leaks memory?

[Maxim Dounin]

Hello!

On Thu, Sep 17, 2009 at 03:45:16PM +0400, Igor Sysoev wrote:

> On Thu, Sep 17, 2009 at 02:40:23PM +0400, Maxim Dounin wrote:
> 
> > Hello!
> > 
> > On Wed, Sep 16, 2009 at 09:24:33AM -0700, Matthew Dempsky wrote:
> > 
> > > On Wed, Sep 16, 2009 at 3:35 AM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> > > > Patch looks correct for me.  It looks a bit fragile though,
> > > > probably we need a bit more bulletproof code here.
> > > 
> > > Would you mind elaborating on what you think is fragile about it?
> > > E.g., how would you rather this bug be patched?
> > 
> > I personally prefer something that won't explode once "goto 
> > failed;" will be reused somewhere after first ngx_resolver_free(r, 
> > name->data).  Just adding "name->data = NULL;" should be enough 
> > (though it's redundant right now).
> 
> No, the patch is good and I have commited it already.
> 
> > > > More generally - resolver known to leak, and probably requires
> > > > code audit.  It would be fine if you look into it.  I believe
> > > > Artem Bokhan will help with testing (cc'd as I'm not sure he is on
> > > > English list).
> > > 
> > > Great.  I'll spend some time looking at the rest of the code then.
> > 
> > BTW, it looks like CNAME case in the same function will also leak 
> > as it sets rn->u.cname without freeing it first.
> > 
> > Probably we just need something like allocation pools as used in 
> > other parts of nginx code.  No idea why Igor wasn't used them in 
> > resolver.  Igor, could you please explain reasons?
> 
> The pools can not be used here for several reasons:
> 
> 1) there is DNS cache,

Yes, I see, this complicates things (or, alternatively, it's 
memory waste).

> 2) DNS request may be shared by several consumers.

I mean DNS request related pools.  It's more or less clear that 
consumer pools can't be used for this.

Maxim Dounin

> 
> However, I think reference counts should be added here.
> 
> 
> -- 
> Igor Sysoev
> http://sysoev.ru/en/
>