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

[Carlos O'Donell <carlos at redhat dot com>]

On 02/08/2017 10:17 AM, Florian Weimer wrote:
> 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.

OK to checkin with more comments added to the test case.

Thank you for pioneering these kinds of test with the network namespace,
they are going a long way to making all of this code more robust and testable.

> 2017-02-08  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.
> 
> diff --git a/sunrpc/Makefile b/sunrpc/Makefile
> index daf8a28..0249e10 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

OK.

>  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)

OK.

>  
>  $(objpfx)rpcgen: $(addprefix $(objpfx),$(rpcgen-objs))
>  
> diff --git a/sunrpc/svc.c b/sunrpc/svc.c
> index 0aef2b5..03f9630 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;
> -}
> -
> -

OK.

I don't see any need for s != NULL_SVC if we don't run svc_find() anymore.

>  /* 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;

OK, load the value before we free the entire struct.

>  
>    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)

OK, check the local boolean to see if it was 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 0000000..f479928
> --- /dev/null
> +++ b/sunrpc/tst-svc_register.c
> @@ -0,0 +1,264 @@
> +/* Test svc_register/svc_unregister rpcbind interaction.

I think this comment should mention bug 5010 since it's a regression test
directly related to the bug (even if it's also a good generic test).

> +   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 <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>
> +

This needs a comment explaining the purpose of this test and explaining the
general structure e.g. using fork, using X, using Y, to test Z.

> +/* 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");
> +}

OK.

> +
> +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)
> +    };
> +}
> +
> +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);
> +}
> +

Should comment on the arbitrary choices here.

> +enum
> +{
> +  /* Coordinates of our test service.  */
> +  PROGNUM = 123,
> +  VERSNUM = 456,
> +
> +  /* Extension for this test.  */
> +  PROC_GET_STATE_AND_EXIT = 10760
> +};
> +
> +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);
> +    }
> +}
> +
> +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 ();
> +}
> +
> +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;
> +}
> +
> +struct test_server_args
> +{
> +  bool use_rpcbind;
> +  bool use_unregister;
> +};
> +
> +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 ();

OK.

> +
> +  /* 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;
> +      }
> +  }
> +

Should comment on the permutations you are testing and why this is sufficient.

> +  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);
> +
> +          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);
> +            }
> +
> +          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.  */

Is this an arbitrary choice?

> +            usleep (300 * 1000);
> +
> +          struct test_state state = get_test_state ();
> +          if (use_rpcbind)
> +            {
> +              TEST_VERIFY (state.set_called);
> +              if (use_thread || use_unregister)
> +                TEST_VERIFY (state.unset_called);
> +              else
> +                /* This is arguably a bug: Regular process termination
> +                   does not unregister the service with rpcbind.  */
> +                TEST_VERIFY (!state.unset_called);
> +            }
> +          else
> +            {
> +              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>
> 


-- 
Cheers,
Carlos.