diff mbox

Fix resource leak in resolver (bug 19257)

Message ID mvmr3gmun60.fsf@hawking.suse.de
State New, archived
Headers

Commit Message

Andreas Schwab Feb. 9, 2016, 11:40 a.m. UTC 
  The number of currently defined nameservers is stored in ->nscount,
whereas ->_u._ext.nscount is set by __libc_res_nsend only after local
initializations.

Andreas.

	* resolv/res_init.c (__res_iclose): Use statp->nscount instead of
	statp->_u._ext.nscount as loop count.
---
 resolv/res_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Florian Weimer Feb. 9, 2016, 12:47 p.m. UTC | #1 
On 02/09/2016 12:40 PM, Andreas Schwab wrote:
> The number of currently defined nameservers is stored in ->nscount,
> whereas ->_u._ext.nscount is set by __libc_res_nsend only after local
> initializations.

Is it possible to test this without a chroot?

Could you please update the nscount comments in resolv.h?

Thanks,
Florian
Andreas Schwab Feb. 9, 2016, 1 p.m. UTC | #2 
Florian Weimer <fweimer@redhat.com> writes:

> On 02/09/2016 12:40 PM, Andreas Schwab wrote:
>> The number of currently defined nameservers is stored in ->nscount,
>> whereas ->_u._ext.nscount is set by __libc_res_nsend only after local
>> initializations.
>
> Is it possible to test this without a chroot?

I don't know how.  You need at least one IPv6 nameserver in
/etc/resolv.conf to trigger the leak.

Andreas.
Andreas Schwab Feb. 9, 2016, 1:02 p.m. UTC | #3 
Florian Weimer <fweimer@redhat.com> writes:

> Could you please update the nscount comments in resolv.h?

I think the comment is accurate.

Andreas.
Florian Weimer Feb. 9, 2016, 1:07 p.m. UTC | #4 
On 02/09/2016 02:02 PM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> 
>> Could you please update the nscount comments in resolv.h?
> 
> I think the comment is accurate.

The comments do not explain the relationship between nscount and
_ext.nscount (_ext.nscount is completely undocumented in the header file).

I just want to avoid future mistakes of this kind.

Florian
Florian Weimer Feb. 9, 2016, 1:10 p.m. UTC | #5 
On 02/09/2016 02:00 PM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> 
>> On 02/09/2016 12:40 PM, Andreas Schwab wrote:
>>> The number of currently defined nameservers is stored in ->nscount,
>>> whereas ->_u._ext.nscount is set by __libc_res_nsend only after local
>>> initializations.
>>
>> Is it possible to test this without a chroot?
> 
> I don't know how.  You need at least one IPv6 nameserver in
> /etc/resolv.conf to trigger the leak.

I tried static linking to override _IO_new_fopen, but I get this:

/home/fweimer/src/gnu/glibc/build/libc.a(iofopen.o): In function
`_IO_new_fopen':
/home/fweimer/src/gnu/glibc/git/libio/iofopen.c:97: multiple definition
of `_IO_new_fopen'
t.o:/tmp/t.c:14: first defined here

Any other ideas?

Thanks,
Florian
Andreas Schwab Feb. 9, 2016, 1:11 p.m. UTC | #6 
Florian Weimer <fweimer@redhat.com> writes:

> I just want to avoid future mistakes of this kind.

The only future is to rewrite the resolver from scratch.

Andreas.
Andreas Schwab Feb. 9, 2016, 1:21 p.m. UTC | #7 
Florian Weimer <fweimer@redhat.com> writes:

> I tried static linking to override _IO_new_fopen, but I get this:
>
> /home/fweimer/src/gnu/glibc/build/libc.a(iofopen.o): In function
> `_IO_new_fopen':
> /home/fweimer/src/gnu/glibc/git/libio/iofopen.c:97: multiple definition
> of `_IO_new_fopen'
> t.o:/tmp/t.c:14: first defined here

Why do you think this is going to work?

Andreas.
Florian Weimer Feb. 9, 2016, 1:31 p.m. UTC | #8 
On 02/09/2016 02:21 PM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> 
>> I tried static linking to override _IO_new_fopen, but I get this:
>>
>> /home/fweimer/src/gnu/glibc/build/libc.a(iofopen.o): In function
>> `_IO_new_fopen':
>> /home/fweimer/src/gnu/glibc/git/libio/iofopen.c:97: multiple definition
>> of `_IO_new_fopen'
>> t.o:/tmp/t.c:14: first defined here
> 
> Why do you think this is going to work?

I think it would work if the file which defined _IO_new_fopen is not
pulled into the link for other reasons.

Florian
Andreas Schwab Feb. 9, 2016, 1:41 p.m. UTC | #9 
Florian Weimer <fweimer@redhat.com> writes:

> I think it would work if the file which defined _IO_new_fopen is not
> pulled into the link for other reasons.

You need to provide at least all aliases.

Andreas.
Florian Weimer Feb. 14, 2016, 11:42 a.m. UTC | #10 
On 02/09/2016 02:00 PM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> 
>> On 02/09/2016 12:40 PM, Andreas Schwab wrote:
>>> The number of currently defined nameservers is stored in ->nscount,
>>> whereas ->_u._ext.nscount is set by __libc_res_nsend only after local
>>> initializations.
>>
>> Is it possible to test this without a chroot?
> 
> I don't know how.  You need at least one IPv6 nameserver in
> /etc/resolv.conf to trigger the leak.

I posted a patch to help with testing:

  https://sourceware.org/ml/libc-alpha/2016-02/msg00376.html

Florian
Carlos O'Donell Feb. 15, 2016, 1:17 a.m. UTC | #11 
On 02/09/2016 08:11 AM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> 
>> I just want to avoid future mistakes of this kind.
> 
> The only future is to rewrite the resolver from scratch.

I would always like to see incremental progress for bugs we fix,
and that includes adding enough comments to prevent future regressions
(which may be on release branches which will never get a rewrite).

Cheers,
Carlos.
Aurelien Jarno March 14, 2016, 7:10 p.m. UTC | #12 
On 2016-02-09 12:40, Andreas Schwab wrote:
> The number of currently defined nameservers is stored in ->nscount,
> whereas ->_u._ext.nscount is set by __libc_res_nsend only after local
> initializations.
> 
> Andreas.
> 
> 	* resolv/res_init.c (__res_iclose): Use statp->nscount instead of
> 	statp->_u._ext.nscount as loop count.
> ---
>  resolv/res_init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/resolv/res_init.c b/resolv/res_init.c
> index e0b6a80..6c951f5 100644
> --- a/resolv/res_init.c
> +++ b/resolv/res_init.c
> @@ -594,7 +594,7 @@ __res_iclose(res_state statp, bool free_addr) {
>  		statp->_vcsock = -1;
>  		statp->_flags &= ~(RES_F_VC | RES_F_CONN);
>  	}
> -	for (ns = 0; ns < statp->_u._ext.nscount; ns++)
> +	for (ns = 0; ns < statp->nscount; ns++)
>  		if (statp->_u._ext.nsaddrs[ns]) {
>  			if (statp->_u._ext.nssocks[ns] != -1) {
>  				close_not_cancel_no_status(statp->_u._ext.nssocks[ns]);

Thanks for the patch. I confirm this is the right thing to do, and I
have tested that it works as expected.
Florian Weimer March 14, 2016, 7:58 p.m. UTC | #13 
On 03/14/2016 08:10 PM, Aurelien Jarno wrote:
> On 2016-02-09 12:40, Andreas Schwab wrote:
>> The number of currently defined nameservers is stored in ->nscount,
>> whereas ->_u._ext.nscount is set by __libc_res_nsend only after local
>> initializations.
>>
>> Andreas.
>>
>> 	* resolv/res_init.c (__res_iclose): Use statp->nscount instead of
>> 	statp->_u._ext.nscount as loop count.
>> ---
>>  resolv/res_init.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/resolv/res_init.c b/resolv/res_init.c
>> index e0b6a80..6c951f5 100644
>> --- a/resolv/res_init.c
>> +++ b/resolv/res_init.c
>> @@ -594,7 +594,7 @@ __res_iclose(res_state statp, bool free_addr) {
>>  		statp->_vcsock = -1;
>>  		statp->_flags &= ~(RES_F_VC | RES_F_CONN);
>>  	}
>> -	for (ns = 0; ns < statp->_u._ext.nscount; ns++)
>> +	for (ns = 0; ns < statp->nscount; ns++)
>>  		if (statp->_u._ext.nsaddrs[ns]) {
>>  			if (statp->_u._ext.nssocks[ns] != -1) {
>>  				close_not_cancel_no_status(statp->_u._ext.nssocks[ns]);
> 
> Thanks for the patch. I confirm this is the right thing to do, and I
> have tested that it works as expected.

Andreas, can you commit this, please?

I'll add a test for it later.

Florian
diff mbox

Patch

diff --git a/resolv/res_init.c b/resolv/res_init.c
index e0b6a80..6c951f5 100644
--- a/resolv/res_init.c
+++ b/resolv/res_init.c
@@ -594,7 +594,7 @@  __res_iclose(res_state statp, bool free_addr) {
 		statp->_vcsock = -1;
 		statp->_flags &= ~(RES_F_VC | RES_F_CONN);
 	}
-	for (ns = 0; ns < statp->_u._ext.nscount; ns++)
+	for (ns = 0; ns < statp->nscount; ns++)
 		if (statp->_u._ext.nsaddrs[ns]) {
 			if (statp->_u._ext.nssocks[ns] != -1) {
 				close_not_cancel_no_status(statp->_u._ext.nssocks[ns]);