gai_cancel()

[Phil Blundell <pb at pbcl dot net>]

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 <pb@pbcl.net>
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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <mcheck.h>
+#include <netdb.h>
+#include <resolv.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+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 <test-skeleton.c>
-- 
2.11.0