sunrpc: Do not unregister services if not registered [BZ #5010]

The change in commit 718946816cf60374f9d8f674d3ed649fdb33205a
has no effect because of two bugs which cancel each other out:
The svc_is_mapped condition is inverted, and svc_is_mapped
always returns false because the check is performed after
the service has already been unregistered.  As a result,
pmap_unset is called unconditionally, as before.

4 files changed, 316 insertions, 12 deletions

diff --git a/ChangeLog b/ChangeLog
index 19ec5b2ec5..edccd36002 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2017-02-28  Florian Weimer  <fweimer@redhat.com>
+
+	[BZ #5010]
+	* sunrpc/svc.c (svc_is_mapped): Remove.
+	(svc_unregister): Obtain mapped status while the service is still
+	registered.
+	* sunrpc/Makefile [have-thread-library] (tests): Add
+	tst-svc_register.
+	(tst-svc_register): Link against libc.so explicitly and the thread
+	library.
+	* sunrpc/tst-svc_register.c: New file.
+
 2017-02-28  Andreas Schwab  <schwab@linux-m68k.org>
 
 	* bits/sigthread.h: Refer to <signal.h>, not <pthread.h>.
diff --git a/sunrpc/Makefile b/sunrpc/Makefile
index daf8a28f1a..0249e10545 100644
--- a/sunrpc/Makefile
+++ b/sunrpc/Makefile
@@ -98,6 +98,7 @@ xtests := tst-getmyaddr
 
 ifeq ($(have-thread-library),yes)
 xtests += thrsvc
+tests += tst-svc_register
 endif
 
 ifeq ($(run-built-tests),yes)
@@ -156,6 +157,8 @@ $(objpfx)tst-getmyaddr: $(common-objpfx)linkobj/libc.so
 $(objpfx)tst-xdrmem: $(common-objpfx)linkobj/libc.so
 $(objpfx)tst-xdrmem2: $(common-objpfx)linkobj/libc.so
 $(objpfx)tst-udp-error: $(common-objpfx)linkobj/libc.so
+$(objpfx)tst-svc_register: \
+  $(common-objpfx)linkobj/libc.so $(shared-thread-library)
 
 $(objpfx)rpcgen: $(addprefix $(objpfx),$(rpcgen-objs))
 
diff --git a/sunrpc/svc.c b/sunrpc/svc.c
index 0aef2b5f95..03f963018c 100644
--- a/sunrpc/svc.c
+++ b/sunrpc/svc.c
@@ -182,17 +182,6 @@ done:
   return s;
 }
 
-
-static bool_t
-svc_is_mapped (rpcprog_t prog, rpcvers_t vers)
-{
-  struct svc_callout *prev;
-  register struct svc_callout *s;
-  s = svc_find (prog, vers, &prev);
-  return s!= NULL_SVC && s->sc_mapped;
-}
-
-
 /* Add a service program to the callout list.
    The dispatch routine will be called when a rpc request for this
    program number comes in. */
@@ -248,6 +237,7 @@ svc_unregister (rpcprog_t prog, rpcvers_t vers)
 
   if ((s = svc_find (prog, vers, &prev)) == NULL_SVC)
     return;
+  bool is_mapped = s->sc_mapped;
 
   if (prev == NULL_SVC)
     svc_head = s->sc_next;
@@ -257,7 +247,7 @@ svc_unregister (rpcprog_t prog, rpcvers_t vers)
   s->sc_next = NULL_SVC;
   mem_free ((char *) s, (u_int) sizeof (struct svc_callout));
   /* now unregister the information with the local binder service */
-  if (! svc_is_mapped (prog, vers))
+  if (is_mapped)
     pmap_unset (prog, vers);
 }
 libc_hidden_nolink_sunrpc (svc_unregister, GLIBC_2_0)
diff --git a/sunrpc/tst-svc_register.c b/sunrpc/tst-svc_register.c
new file mode 100644
index 0000000000..8934094c99
--- /dev/null
+++ b/sunrpc/tst-svc_register.c
@@ -0,0 +1,299 @@
+/* Test svc_register/svc_unregister rpcbind interaction (bug 5010).
+   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/>.  */
+
+/* This test uses a stub rpcbind server (implemented in a child
+   process using rpcbind_dispatch/run_rpcbind) to check how RPC
+   services are registered and unregistered using the rpcbind
+   protocol.  For each subtest, a separate rpcbind test server is
+   spawned and terminated.  */
+
+#include <errno.h>
+#include <netinet/in.h>
+#include <rpc/clnt.h>
+#include <rpc/pmap_prot.h>
+#include <rpc/svc.h>
+#include <signal.h>
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/test-driver.h>
+#include <support/xsocket.h>
+#include <support/xthread.h>
+#include <support/xunistd.h>
+#include <sys/socket.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include <libc-symbols.h>
+#include <shlib-compat.h>
+
+/* These functions are only available as compat symbols.  */
+compat_symbol_reference (libc, xdr_pmap, xdr_pmap, GLIBC_2_0);
+compat_symbol_reference (libc, svc_unregister, svc_unregister, GLIBC_2_0);
+
+/* Server callback for the unused RPC service which is registered and
+   unregistered.  */
+static void
+server_dispatch (struct svc_req *request, SVCXPRT *transport)
+{
+  FAIL_EXIT1 ("server_dispatch called");
+}
+
+/* The port on which rpcbind listens for incoming requests.  */
+static inline const struct sockaddr_in
+rpcbind_address (void)
+{
+  return (struct sockaddr_in)
+    {
+      .sin_family = AF_INET,
+      .sin_addr.s_addr = htonl (INADDR_LOOPBACK),
+      .sin_port = htons (PMAPPORT)
+    };
+}
+
+/* Data provided by the test server after running the test, to see
+   that the expected calls (and only those) happened.  */
+struct test_state
+{
+  bool_t set_called;
+  bool_t unset_called;
+};
+
+static bool_t
+xdr_test_state (XDR *xdrs, void *data, ...)
+{
+  struct test_state *p = data;
+  return xdr_bool (xdrs, &p->set_called)
+    && xdr_bool (xdrs, &p->unset_called);
+}
+
+enum
+{
+  /* Coordinates of our test service.  These numbers are
+     arbitrary.  */
+  PROGNUM = 123,
+  VERSNUM = 456,
+
+  /* Extension for this test.  */
+  PROC_GET_STATE_AND_EXIT = 10760
+};
+
+/* Dummy implementation of the rpcbind service, with the
+   PROC_GET_STATE_AND_EXIT extension.  */
+static void
+rpcbind_dispatch (struct svc_req *request, SVCXPRT *transport)
+{
+  static struct test_state state = { 0, };
+
+  if (test_verbose)
+    printf ("info: rpcbind request %lu\n", request->rq_proc);
+
+  switch (request->rq_proc)
+    {
+    case PMAPPROC_SET:
+    case PMAPPROC_UNSET:
+      TEST_VERIFY (state.set_called == (request->rq_proc == PMAPPROC_UNSET));
+      TEST_VERIFY (!state.unset_called);
+
+      struct pmap query = { 0, };
+      TEST_VERIFY
+        (svc_getargs (transport, (xdrproc_t) xdr_pmap, (void *) &query));
+      if (test_verbose)
+        printf ("  pm_prog=%lu pm_vers=%lu pm_prot=%lu pm_port=%lu\n",
+                query.pm_prog, query.pm_vers, query.pm_prot, query.pm_port);
+      TEST_VERIFY (query.pm_prog == PROGNUM);
+      TEST_VERIFY (query.pm_vers == VERSNUM);
+
+      if (request->rq_proc == PMAPPROC_SET)
+        state.set_called = TRUE;
+      else
+        state.unset_called = TRUE;
+
+      bool_t result = TRUE;
+      TEST_VERIFY (svc_sendreply (transport,
+                                  (xdrproc_t) xdr_bool, (void *) &result));
+      break;
+
+    case PROC_GET_STATE_AND_EXIT:
+      TEST_VERIFY (svc_sendreply (transport,
+                                  xdr_test_state, (void *) &state));
+      _exit (0);
+      break;
+
+    default:
+      FAIL_EXIT1 ("invalid rq_proc value: %lu", request->rq_proc);
+    }
+}
+
+/* Run the rpcbind test server.  */
+static void
+run_rpcbind (int rpcbind_sock)
+{
+  SVCXPRT *rpcbind_transport = svcudp_create (rpcbind_sock);
+  TEST_VERIFY (svc_register (rpcbind_transport, PMAPPROG, PMAPVERS,
+                             rpcbind_dispatch,
+                             /* Do not register with rpcbind.  */
+                             0));
+  svc_run ();
+}
+
+/* Call out to the rpcbind test server to retrieve the test status
+   information.  */
+static struct test_state
+get_test_state (void)
+{
+  int socket = RPC_ANYSOCK;
+  struct sockaddr_in address = rpcbind_address ();
+  CLIENT *client = clntudp_create
+    (&address, PMAPPROG, PMAPVERS, (struct timeval) { 1, 0}, &socket);
+  struct test_state result = { 0 };
+  TEST_VERIFY (clnt_call (client, PROC_GET_STATE_AND_EXIT,
+                          (xdrproc_t) xdr_void, NULL,
+                          xdr_test_state, (void *) &result,
+                          ((struct timeval) { 3, 0}))
+               == RPC_SUCCESS);
+  clnt_destroy (client);
+  return result;
+}
+
+/* Used by test_server_thread to receive test parameters.  */
+struct test_server_args
+{
+  bool use_rpcbind;
+  bool use_unregister;
+};
+
+/* RPC test server.  Used to verify the svc_unregister behavior during
+   thread cleanup.  */
+static void *
+test_server_thread (void *closure)
+{
+  struct test_server_args *args = closure;
+  SVCXPRT *transport = svcudp_create (RPC_ANYSOCK);
+  int protocol;
+  if (args->use_rpcbind)
+    protocol = IPPROTO_UDP;
+  else
+    /* Do not register with rpcbind.  */
+    protocol = 0;
+  TEST_VERIFY (svc_register (transport, PROGNUM, VERSNUM,
+                             server_dispatch, protocol));
+  if (args->use_unregister)
+    svc_unregister (PROGNUM, VERSNUM);
+  SVC_DESTROY (transport);
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  support_become_root ();
+  support_enter_network_namespace ();
+
+  /* Try to bind to the rpcbind port.  */
+  int rpcbind_sock = xsocket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+  {
+    struct sockaddr_in sin = rpcbind_address ();
+    if (bind (rpcbind_sock, (struct sockaddr *) &sin, sizeof (sin)) != 0)
+      {
+        /* If the port is not available, we cannot run this test.  */
+        printf ("warning: could not bind to rpcbind port %d: %m\n",
+                (int) PMAPPORT);
+        return EXIT_UNSUPPORTED;
+      }
+  }
+
+  for (int use_thread = 0; use_thread < 2; ++use_thread)
+    for (int use_rpcbind = 0; use_rpcbind < 2; ++use_rpcbind)
+      for (int use_unregister = 0; use_unregister < 2; ++use_unregister)
+        {
+          if (test_verbose)
+            printf ("info: * use_thread=%d use_rpcbind=%d use_unregister=%d\n",
+                    use_thread, use_rpcbind, use_unregister);
+
+          /* Create the subprocess which runs the actual test.  The
+             kernel will queue the UDP packets to the rpcbind
+             process.  */
+          pid_t svc_pid = xfork ();
+          if (svc_pid == 0)
+            {
+              struct test_server_args args =
+                {
+                  .use_rpcbind = use_rpcbind,
+                  .use_unregister = use_unregister,
+                };
+              if (use_thread)
+                xpthread_join (xpthread_create
+                               (NULL, test_server_thread, &args));
+              else
+                test_server_thread (&args);
+              /* We cannnot use _exit here because we want to test the
+                 process cleanup.  */
+              exit (0);
+            }
+
+          /* Create the subprocess for the rpcbind test server.  */
+          pid_t rpcbind_pid = xfork ();
+          if (rpcbind_pid == 0)
+            run_rpcbind (rpcbind_sock);
+
+          int status;
+          xwaitpid (svc_pid, &status, 0);
+          TEST_VERIFY (WIFEXITED (status) && WEXITSTATUS (status) == 0);
+
+          if (!use_rpcbind)
+            /* Wait a bit, to see if the packet arrives on the rpcbind
+               port.  The choice is of the timeout is arbitrary, but
+               should be long enough even for slow/busy systems.  For
+               the use_rpcbind case, waiting on svc_pid above makes
+               sure that the test server has responded because
+               svc_register/svc_unregister are supposed to wait for a
+               reply.  */
+            usleep (300 * 1000);
+
+          struct test_state state = get_test_state ();
+          if (use_rpcbind)
+            {
+              TEST_VERIFY (state.set_called);
+              if (use_thread || use_unregister)
+                /* Thread cleanup or explicit svc_unregister will
+                   result in a rpcbind unset RPC call.  */
+                TEST_VERIFY (state.unset_called);
+              else
+                /* This is arguably a bug: Regular process termination
+                   does not unregister the service with rpcbind.  The
+                   unset rpcbind call happens from a __libc_subfreeres
+                   callback, and this only happens when running under
+                   memory debuggers such as valgrind.  */
+                TEST_VERIFY (!state.unset_called);
+            }
+          else
+            {
+              /* If rpcbind registration is not requested, we do not
+                 expect any rpcbind calls.  */
+              TEST_VERIFY (!state.set_called);
+              TEST_VERIFY (!state.unset_called);
+            }
+
+          xwaitpid (rpcbind_pid, &status, 0);
+          TEST_VERIFY (WIFEXITED (status) && WEXITSTATUS (status) == 0);
+        }
+
+  return 0;
+}
+
+#include <support/test-driver.c>