I’m still familiarizing myself with nginx’s architecture, but it looks
like ngx_resolver.c leaks memory in a few places. Before I spend any
time tracking more down, I thought I’d point out one apparent leak so
someone can perhaps correct me before I waste my time.
In ngx_resolver_process_a(), ngx_resolver_copy() is used to
dynamically allocate memory into name.data. This memory is only
passed to a few logging functions and to ngx_resolver_lookup_name().
(The name variable is also reused in the CNAME handling path, but it’s
first overwritten by another call to ngx_resolver_copy().)
There does not appear to be any code responsible for ensuring that
name.data is freed. Is there any deeper architecture preventing this
memory from being leaked that I’ve missed?
Included below is a patch that I believe fixes this particular memory
leak. If someone can confirm that this is indeed correct, I’ll
continue investigating the other apparent leaks.
Thanks!
— src/core/ngx_resolver.c.orig 2009-09-15 22:12:31.000000000 -0700
+++ src/core/ngx_resolver.c 2009-09-15 22:24:06.000000000 -0700
@@ -1149,6 +1149,8 @@ ngx_resolver_process_a(ngx_resolver_t *r
goto failed;
}
-
ngx_resolver_free(r, name.data);
-
if (code == 0 && nan == 0) {
code = 3; /* NXDOMAIN */
}
@@ -1400,6 +1402,8 @@ failed:
/* unlock name mutex */
-
ngx_resolver_free(r, name.data);
-
return;
}
Hello!
On Tue, Sep 15, 2009 at 10:28:37PM -0700, Matthew Dempsky wrote:
There does not appear to be any code responsible for ensuring that
name.data is freed. Is there any deeper architecture preventing this
memory from being leaked that I’ve missed?
Included below is a patch that I believe fixes this particular memory
leak. If someone can confirm that this is indeed correct, I’ll
continue investigating the other apparent leaks.
Patch looks correct for me. It looks a bit fragile though,
probably we need a bit more bulletproof code here.
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).
Maxim D.
On Tue, Sep 15, 2009 at 10:28:37PM -0700, Matthew Dempsky wrote:
There does not appear to be any code responsible for ensuring that
name.data is freed. Is there any deeper architecture preventing this
memory from being leaked that I’ve missed?
Included below is a patch that I believe fixes this particular memory
leak. If someone can confirm that this is indeed correct, I’ll
continue investigating the other apparent leaks.
You are right, thank you.
On Wed, Sep 16, 2009 at 3:35 AM, Maxim D. [email protected]
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?
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.
Thanks.
Hello!
On Wed, Sep 16, 2009 at 09:24:33AM -0700, Matthew Dempsky wrote:
On Wed, Sep 16, 2009 at 3:35 AM, Maxim D. [email protected] 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).
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?
Maxim D.
On Thu, Sep 17, 2009 at 02:40:23PM +0400, Maxim D. wrote:
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.
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:
- there is DNS cache,
- DNS request may be shared by several consumers.
However, I think reference counts should be added here.
Hello!
On Thu, Sep 17, 2009 at 03:45:16PM +0400, Igor S. wrote:
Would you mind elaborating on what you think is fragile about it?
code audit. šIt would be fine if you look into it. šI believe
resolver. Igor, could you please explain reasons?
The pools can not be used here for several reasons:
- there is DNS cache,
Yes, I see, this complicates things (or, alternatively, it’s
memory waste).
- 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 D.