From patchwork Fri Jun 16 15:46:24 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Phil Blundell X-Patchwork-Id: 21051 Received: (qmail 68169 invoked by alias); 16 Jun 2017 15:46:38 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 53614 invoked by uid 89); 16 Jun 2017 15:46:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: =?ISO-8859-1?Q?No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=the, the=c2, H*F:U*pb, 1078?= X-HELO: hetzner.pbcl.net Message-ID: <1497627984.6717.32.camel@pbcl.net> Subject: gai_cancel() From: Phil Blundell To: libc-alpha@sourceware.org Date: Fri, 16 Jun 2017 16:46:24 +0100 Mime-Version: 1.0 According to the Linux manpage for getaddrinfo_a(), the behaviour of gai_cancel() is as follows: "The gai_cancel() function cancels the request req.  If the request has been  canceled  successfully,  the  error  status  of  the request will be set to EAI_CANCELED and normal asynchronous notification will be performed." However, this doesn't seem to match the actual behaviour. From my tests and inspection of the code, if a request is successfully cancelled then asynchronous notification is not performed: the request is simply dropped from the queue.  This wouldn't in itself be unreasonable because the application can tell from the return value whether or not this happened and deal with it appropriately. Initially I assumed that the code as written was correct and the documentation was simply missing a crucial "not". But, it further turns out that if notification is not performed, and hence __gai_notify is not called, the async_waitlist struct that was allocated by getaddrinfo_a() is leaked. I've attached a testcase that demonstrates the leak. Clearly, changing gai_cancel() to perform notification would make the code match the documentation and would also fix the memory leak. But it doesn't seem impossible that there might be applications which are now relying on the current behaviour and might break if this change were made. Since getaddrinfo_a() doesn't appear to be part of any standard, and it also isn't documented in the glibc manual, I wasn't able to find any other reference that describes what the behaviour is supposed to be. Opinions? thanks p. From 11ec70a0bcd3bef4595020ada88303d349884f22 Mon Sep 17 00:00:00 2001 From: Phil Blundell Date: Fri, 16 Jun 2017 16:36:52 +0100 Subject: [PATCH] resolv/tst-leaks3.c: New test --- resolv/Makefile | 15 +++++++++++---- resolv/tst-leaks3.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 4 deletions(-) create mode 100644 resolv/tst-leaks3.c diff --git a/resolv/Makefile b/resolv/Makefile index dc597ca097..22926a9e5b 100644 --- a/resolv/Makefile +++ b/resolv/Makefile @@ -31,9 +31,9 @@ routines := herror inet_addr inet_ntop inet_pton nsap_addr res_init \ res_hconf res_libc res-state tests = tst-aton tst-leaks tst-inet_ntop -xtests = tst-leaks2 +xtests = tst-leaks2 tst-leaks3 -generate := mtrace-tst-leaks.out tst-leaks.mtrace tst-leaks2.mtrace +generate := mtrace-tst-leaks.out tst-leaks.mtrace tst-leaks2.mtrace tst-leaks3.mtrace extra-libs := libresolv libnss_dns ifeq ($(have-thread-library),yes) @@ -95,7 +95,7 @@ endif ifeq ($(run-built-tests),yes) ifneq (no,$(PERL)) tests-special += $(objpfx)mtrace-tst-leaks.out -xtests-special += $(objpfx)mtrace-tst-leaks2.out +xtests-special += $(objpfx)mtrace-tst-leaks2.out $(objpfx)mtrace-tst-leaks3.out endif endif @@ -107,7 +107,8 @@ headers += rpc/netdb.h endif generated += mtrace-tst-leaks.out tst-leaks.mtrace \ - mtrace-tst-leaks2.out tst-leaks2.mtrace + mtrace-tst-leaks2.out tst-leaks2.mtrace \ + mtrace-tst-leaks3.out tst-leaks3.mtrace include ../Rules @@ -135,6 +136,12 @@ $(objpfx)mtrace-tst-leaks2.out: $(objpfx)tst-leaks2.out $(common-objpfx)malloc/mtrace $(objpfx)tst-leaks2.mtrace > $@; \ $(evaluate-test) +$(objpfx)tst-leaks3: $(objpfx)libresolv.so $(objpfx)libanl.so +tst-leaks3-ENV = MALLOC_TRACE=$(objpfx)tst-leaks3.mtrace +$(objpfx)mtrace-tst-leaks3.out: $(objpfx)tst-leaks3.out + $(common-objpfx)malloc/mtrace $(objpfx)tst-leaks3.mtrace > $@; \ + $(evaluate-test) + $(objpfx)tst-bug18665-tcp: $(objpfx)libresolv.so $(shared-thread-library) $(objpfx)tst-bug18665: $(objpfx)libresolv.so $(shared-thread-library) $(objpfx)tst-res_use_inet6: $(objpfx)libresolv.so $(shared-thread-library) diff --git a/resolv/tst-leaks3.c b/resolv/tst-leaks3.c new file mode 100644 index 0000000000..93f06e9e84 --- /dev/null +++ b/resolv/tst-leaks3.c @@ -0,0 +1,54 @@ +/* Tests for getaddrinfo_a + Copyright (C) 2017 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#include +#include +#include +#include +#include +#include + +static int +do_test (void) +{ + mtrace (); + + for (int i = 0; i < 1000; ++i) + { + struct gaicb *req[1]; + req[0] = malloc (sizeof (*req[0])); + memset (req[0], 0, sizeof (*req[0])); + req[0]->ar_name = "www.gnu.org"; + + int ret = getaddrinfo_a (GAI_NOWAIT, req, 1, NULL); + if (ret != 0) + { + fprintf (stderr, "getaddrinfo_a() failed: %s\n", + gai_strerror(ret)); + exit (EXIT_FAILURE); + } + + gai_cancel (req[0]); + } + return 0; +} + +#define TEST_FUNCTION do_test () +#define TIMEOUT 30 +/* This defines the `main' function and some more. */ +#include -- 2.11.0