Message ID | mvmr3gmun60.fsf@hawking.suse.de |
---|---|
State | New, archived |
Headers |
Received: (qmail 71746 invoked by alias); 9 Feb 2016 11:40:48 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 71558 invoked by uid 89); 9 Feb 2016 11:40:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx2.suse.de From: Andreas Schwab <schwab@suse.de> To: libc-alpha@sourceware.org Subject: [PATCH] Fix resource leak in resolver (bug 19257) X-Yow: Okay, BARBRA STREISAND, I recognize you now!! Also EFREM ZIMBALIST, JUNIOR!! And BEAUMONT NEWHALL!! Everybody into th' BATHROOM! Date: Tue, 09 Feb 2016 12:40:39 +0100 Message-ID: <mvmr3gmun60.fsf@hawking.suse.de> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain |
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
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
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.
Florian Weimer <fweimer@redhat.com> writes:
> Could you please update the nscount comments in resolv.h?
I think the comment is accurate.
Andreas.
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
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
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.
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.
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
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.
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
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.
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.
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 --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]);