sunrpc: Improvements for UDP client timeout handling [BZ #20257]

This commit fixes various aspects in the UDP client timeout handling.
Timeouts are now applied in a more consistent fashion.  Discarded UDP
packets no longer prevent the timeout from happening at all.

5 files changed, 919 insertions, 57 deletions

diff --git a/sunrpc/Makefile b/sunrpc/Makefile
index 0249e10545..063a76161c 100644
--- a/sunrpc/Makefile
+++ b/sunrpc/Makefile
@@ -93,12 +93,13 @@ rpcgen-objs = rpc_main.o rpc_hout.o rpc_cout.o rpc_parse.o \
 extra-objs = $(rpcgen-objs) $(addprefix cross-,$(rpcgen-objs))
 others += rpcgen
 
-tests = tst-xdrmem tst-xdrmem2 test-rpcent tst-udp-error
+tests = tst-xdrmem tst-xdrmem2 test-rpcent tst-udp-error tst-udp-timeout \
+  tst-udp-nonblocking
 xtests := tst-getmyaddr
 
 ifeq ($(have-thread-library),yes)
 xtests += thrsvc
-tests += tst-svc_register
+tests += tst-svc_register tst-udp-garbage
 endif
 
 ifeq ($(run-built-tests),yes)
@@ -238,3 +239,8 @@ $(rpcgen-tests): $(objpfx)%.out: %.x $(objpfx)rpcgen
 	$(built-program-cmd) -c $< -o $@; \
 	$(evaluate-test)
 endif
+
+$(objpfx)tst-udp-timeout: $(common-objpfx)linkobj/libc.so
+$(objpfx)tst-udp-nonblocking: $(common-objpfx)linkobj/libc.so
+$(objpfx)tst-udp-garbage: \
+  $(common-objpfx)linkobj/libc.so $(shared-thread-library)
diff --git a/sunrpc/clnt_udp.c b/sunrpc/clnt_udp.c
index 1de25cb771..6ce16eb298 100644
--- a/sunrpc/clnt_udp.c
+++ b/sunrpc/clnt_udp.c
@@ -55,6 +55,7 @@
 #endif
 
 #include <kernel-features.h>
+#include <inet/net-internal.h>
 
 extern u_long _create_xid (void);
 
@@ -80,7 +81,9 @@ static const struct clnt_ops udp_ops =
 };
 
 /*
- * Private data kept per client handle
+ * Private data kept per client handle.  This private struct is
+ * unfortunately part of the ABI; ypbind contains a copy of it and
+ * accesses it through CLIENT::cl_private field.
  */
 struct cu_data
   {
@@ -278,28 +281,38 @@ clntudp_call (/* client handle */
   int inlen;
   socklen_t fromlen;
   struct pollfd fd;
-  int milliseconds = (cu->cu_wait.tv_sec * 1000) +
-    (cu->cu_wait.tv_usec / 1000);
   struct sockaddr_in from;
   struct rpc_msg reply_msg;
   XDR reply_xdrs;
-  struct timeval time_waited;
   bool_t ok;
   int nrefreshes = 2;		/* number of times to refresh cred */
-  struct timeval timeout;
   int anyup;			/* any network interface up */
 
-  if (cu->cu_total.tv_usec == -1)
-    {
-      timeout = utimeout;	/* use supplied timeout */
-    }
-  else
+  struct deadline_current_time current_time = __deadline_current_time ();
+  struct deadline total_deadline; /* Determined once by overall timeout.  */
+  struct deadline response_deadline; /* Determined anew for each query.  */
+
+  /* Choose the timeout value.  For non-sending usage (xargs == NULL),
+     the total deadline does not matter, only cu->cu_wait is used
+     below.  */
+  if (xargs != NULL)
     {
-      timeout = cu->cu_total;	/* use default timeout */
+      struct timeval tv;
+      if (cu->cu_total.tv_usec == -1)
+	/* Use supplied timeout.  */
+	tv = utimeout;
+      else
+	/* Use default timeout.  */
+	tv = cu->cu_total;
+      if (!__is_timeval_valid_timeout (tv))
+	return (cu->cu_error.re_status = RPC_TIMEDOUT);
+      total_deadline = __deadline_from_timeval (current_time, tv);
     }
 
-  time_waited.tv_sec = 0;
-  time_waited.tv_usec = 0;
+  /* Guard against bad timeout specification.  */
+  if (!__is_timeval_valid_timeout (cu->cu_wait))
+    return (cu->cu_error.re_status = RPC_TIMEDOUT);
+
 call_again:
   xdrs = &(cu->cu_outxdrs);
   if (xargs == NULL)
@@ -325,27 +338,46 @@ send_again:
       return (cu->cu_error.re_status = RPC_CANTSEND);
     }
 
-  /*
-   * Hack to provide rpc-based message passing
-   */
-  if (timeout.tv_sec == 0 && timeout.tv_usec == 0)
-    {
-      return (cu->cu_error.re_status = RPC_TIMEDOUT);
-    }
+  /* sendto may have blocked, so recompute the current time.  */
+  current_time = __deadline_current_time ();
  get_reply:
-  /*
-   * sub-optimal code appears here because we have
-   * some clock time to spare while the packets are in flight.
-   * (We assume that this is actually only executed once.)
-   */
+  response_deadline = __deadline_from_timeval (current_time, cu->cu_wait);
+
   reply_msg.acpted_rply.ar_verf = _null_auth;
   reply_msg.acpted_rply.ar_results.where = resultsp;
   reply_msg.acpted_rply.ar_results.proc = xresults;
   fd.fd = cu->cu_sock;
   fd.events = POLLIN;
   anyup = 0;
+
+  /* Per-response retry loop.  current_time must be up-to-date at the
+     top of the loop.  */
   for (;;)
     {
+      int milliseconds;
+      if (xargs != NULL)
+	{
+	  if (__deadline_elapsed (current_time, total_deadline))
+	    /* Overall timeout expired.  */
+	    return (cu->cu_error.re_status = RPC_TIMEDOUT);
+	  milliseconds = __deadline_to_ms
+	    (current_time, __deadline_first (total_deadline,
+					     response_deadline));
+	  if (milliseconds == 0)
+	    /* Per-query timeout expired.  */
+	    goto send_again;
+	}
+      else
+	{
+	  /* xatgs == NULL.  Collect a response without sending a
+	     query.  In this mode, we need to ignore the total
+	     deadline.  */
+	  milliseconds = __deadline_to_ms (current_time, response_deadline);
+	  if (milliseconds == 0)
+	    /* Cannot send again, so bail out.  */
+	    return (cu->cu_error.re_status = RPC_CANTSEND);
+	}
+
       switch (__poll (&fd, 1, milliseconds))
 	{
 
@@ -356,27 +388,10 @@ send_again:
 	      if (!anyup)
 		return (cu->cu_error.re_status = RPC_CANTRECV);
 	    }
-
-	  time_waited.tv_sec += cu->cu_wait.tv_sec;
-	  time_waited.tv_usec += cu->cu_wait.tv_usec;
-	  while (time_waited.tv_usec >= 1000000)
-	    {
-	      time_waited.tv_sec++;
-	      time_waited.tv_usec -= 1000000;
-	    }
-	  if ((time_waited.tv_sec < timeout.tv_sec) ||
-	      ((time_waited.tv_sec == timeout.tv_sec) &&
-	       (time_waited.tv_usec < timeout.tv_usec)))
-	    goto send_again;
-	  return (cu->cu_error.re_status = RPC_TIMEDOUT);
-
-	  /*
-	   * buggy in other cases because time_waited is not being
-	   * updated.
-	   */
+	  goto next_response;
 	case -1:
 	  if (errno == EINTR)
-	    continue;
+	    goto next_response;
 	  cu->cu_error.re_errno = errno;
 	  return (cu->cu_error.re_status = RPC_CANTRECV);
 	}
@@ -440,20 +455,22 @@ send_again:
       if (inlen < 0)
 	{
 	  if (errno == EWOULDBLOCK)
-	    continue;
+	    goto next_response;
 	  cu->cu_error.re_errno = errno;
 	  return (cu->cu_error.re_status = RPC_CANTRECV);
 	}
-      if (inlen < 4)
-	continue;
-
-      /* see if reply transaction id matches sent id.
-	Don't do this if we only wait for a replay */
-      if (xargs != NULL
-	  && memcmp (cu->cu_inbuf, cu->cu_outbuf, sizeof (u_int32_t)) != 0)
-	continue;
-      /* we now assume we have the proper reply */
-      break;
+      /* Accept the response if the packet is sufficiently long and
+	 the transaction ID matches the query (if available).  */
+      if (inlen >= 4
+	  && (xargs == NULL
+	      || memcmp (cu->cu_inbuf, cu->cu_outbuf,
+			 sizeof (u_int32_t)) == 0))
+	break;
+
+    next_response:
+      /* Update the current time because poll and recvmsg waited for
+	 an unknown time.  */
+      current_time = __deadline_current_time ();
     }
 
   /*
diff --git a/sunrpc/tst-udp-garbage.c b/sunrpc/tst-udp-garbage.c
new file mode 100644
index 0000000000..4abda93f08
--- /dev/null
+++ b/sunrpc/tst-udp-garbage.c
@@ -0,0 +1,104 @@
+/* Test that garbage packets do not affect timeout handling.
+   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 <netinet/in.h>
+#include <rpc/clnt.h>
+#include <rpc/svc.h>
+#include <stdbool.h>
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/xsocket.h>
+#include <support/xthread.h>
+#include <sys/socket.h>
+#include <unistd.h>
+
+/* Descriptor for the server UDP socket.  */
+static int server_fd;
+
+static void *
+garbage_sender_thread (void *unused)
+{
+  while (true)
+    {
+      struct sockaddr_storage sa;
+      socklen_t salen = sizeof (sa);
+      char buf[1];
+      if (recvfrom (server_fd, buf, sizeof (buf), 0,
+                    (struct sockaddr *) &sa, &salen) < 0)
+        FAIL_EXIT1 ("recvfrom: %m");
+
+      /* Send garbage packets indefinitely.  */
+      buf[0] = 0;
+      while (true)
+        {
+          /* sendto can fail if the client closed the socket.  */
+          if (sendto (server_fd, buf, sizeof (buf), 0,
+                      (struct sockaddr *) &sa, salen) < 0)
+            break;
+
+          /* Wait a bit, to avoid burning too many CPU cycles in a
+             tight loop.  The wait period must be much shorter than
+             the client timeouts configured below.  */
+          usleep (50 * 1000);
+        }
+    }
+}
+
+static int
+do_test (void)
+{
+  support_become_root ();
+  support_enter_network_namespace ();
+
+  server_fd = xsocket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, IPPROTO_UDP);
+  struct sockaddr_in server_address =
+    {
+      .sin_family = AF_INET,
+      .sin_addr.s_addr = htonl (INADDR_LOOPBACK),
+    };
+  xbind (server_fd,
+         (struct sockaddr *) &server_address, sizeof (server_address));
+  {
+    socklen_t sinlen = sizeof (server_address);
+    xgetsockname (server_fd, (struct sockaddr *) &server_address, &sinlen);
+    TEST_VERIFY (sizeof (server_address) == sinlen);
+  }
+
+  /* Garbage packet source.  */
+  xpthread_detach (xpthread_create (NULL, garbage_sender_thread, NULL));
+
+  /* Test client.  Use an arbitrary timeout of one second, which is
+     much longer than the garbage packet interval, but still
+     reasonably short, so that the test completes quickly.  */
+  int client_fd = RPC_ANYSOCK;
+  CLIENT *clnt = clntudp_create (&server_address,
+                                 1, 2, /* Arbitrary RPC endpoint numbers.  */
+                                 (struct timeval) { 1, 0 },
+                                 &client_fd);
+  if (clnt == NULL)
+    FAIL_EXIT1 ("clntudp_create: %m");
+
+  TEST_VERIFY (clnt_call (clnt, 3, /* Arbitrary RPC procedure number.  */
+                          (xdrproc_t) xdr_void, NULL,
+                          (xdrproc_t) xdr_void, NULL,
+                          ((struct timeval) { 1, 0 })));
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sunrpc/tst-udp-nonblocking.c b/sunrpc/tst-udp-nonblocking.c
new file mode 100644
index 0000000000..1d6a7f4b56
--- /dev/null
+++ b/sunrpc/tst-udp-nonblocking.c
@@ -0,0 +1,333 @@
+/* Test non-blocking use of the UDP client.
+   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 <netinet/in.h>
+#include <rpc/clnt.h>
+#include <rpc/svc.h>
+#include <stdbool.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/test-driver.h>
+#include <support/xsocket.h>
+#include <support/xunistd.h>
+#include <sys/socket.h>
+#include <time.h>
+#include <unistd.h>
+
+/* Test data serialization and deserialization.   */
+
+struct test_query
+{
+  uint32_t a;
+  uint32_t b;
+  uint32_t timeout_ms;
+};
+
+static bool_t
+xdr_test_query (XDR *xdrs, void *data, ...)
+{
+  struct test_query *p = data;
+  return xdr_uint32_t (xdrs, &p->a)
+    && xdr_uint32_t (xdrs, &p->b)
+    && xdr_uint32_t (xdrs, &p->timeout_ms);
+}
+
+struct test_response
+{
+  uint32_t server_id;
+  uint32_t seq;
+  uint32_t sum;
+};
+
+static bool_t
+xdr_test_response (XDR *xdrs, void *data, ...)
+{
+  struct test_response *p = data;
+  return xdr_uint32_t (xdrs, &p->server_id)
+    && xdr_uint32_t (xdrs, &p->seq)
+    && xdr_uint32_t (xdrs, &p->sum);
+}
+
+/* Implementation of the test server.  */
+
+enum
+  {
+    /* Number of test servers to run. */
+    SERVER_COUNT = 3,
+
+    /* RPC parameters, chosen at random.  */
+    PROGNUM = 8242,
+    VERSNUM = 19654,
+
+    /* Main RPC operation.  */
+    PROC_ADD = 1,
+
+    /* Request process termination.  */
+    PROC_EXIT,
+
+    /* Special exit status to mark successful processing.  */
+    EXIT_MARKER = 55,
+  };
+
+/* Set by the parent process to tell test servers apart.  */
+static int server_id;
+
+/* Implementation of the test server.  */
+static void
+server_dispatch (struct svc_req *request, SVCXPRT *transport)
+{
+  /* Query sequence number.  */
+  static uint32_t seq = 0;
+  ++seq;
+  static bool proc_add_seen;
+
+  if (test_verbose)
+    printf ("info: server_dispatch server_id=%d seq=%u rq_proc=%lu\n",
+            server_id, seq, request->rq_proc);
+
+  switch (request->rq_proc)
+    {
+    case PROC_ADD:
+      {
+        struct test_query query;
+        memset (&query, 0xc0, sizeof (query));
+        TEST_VERIFY_EXIT
+          (svc_getargs (transport, xdr_test_query,
+                        (void *) &query));
+
+        if (test_verbose)
+          printf ("  a=%u b=%u timeout_ms=%u\n",
+                  query.a, query.b, query.timeout_ms);
+
+        usleep (query.timeout_ms * 1000);
+
+        struct test_response response =
+          {
+            .server_id = server_id,
+            .seq = seq,
+            .sum = query.a + query.b,
+          };
+        TEST_VERIFY (svc_sendreply (transport, xdr_test_response,
+                                    (void *) &response));
+        if (test_verbose)
+          printf ("  server id %d response seq=%u sent\n", server_id, seq);
+        proc_add_seen = true;
+      }
+      break;
+
+    case PROC_EXIT:
+      TEST_VERIFY (proc_add_seen);
+      TEST_VERIFY (svc_sendreply (transport, (xdrproc_t) xdr_void, NULL));
+      _exit (EXIT_MARKER);
+      break;
+
+    default:
+      FAIL_EXIT1 ("invalid rq_proc value: %lu", request->rq_proc);
+      break;
+    }
+}
+
+/* Return the number seconds since an arbitrary point in time.  */
+static double
+get_ticks (void)
+{
+  {
+    struct timespec ts;
+    if (clock_gettime (CLOCK_MONOTONIC, &ts) == 0)
+      return ts.tv_sec + ts.tv_nsec * 1e-9;
+  }
+  {
+    struct timeval tv;
+    TEST_VERIFY_EXIT (gettimeofday (&tv, NULL) == 0);
+    return tv.tv_sec + tv.tv_usec * 1e-6;
+  }
+}
+
+static int
+do_test (void)
+{
+  support_become_root ();
+  support_enter_network_namespace ();
+
+  /* Information about the test servers.  */
+  struct
+  {
+    SVCXPRT *transport;
+    struct sockaddr_in address;
+    pid_t pid;
+    uint32_t xid;
+  } servers[SERVER_COUNT];
+
+  /* Spawn the test servers.  */
+  for (int i = 0; i < SERVER_COUNT; ++i)
+    {
+      servers[i].transport = svcudp_create (RPC_ANYSOCK);
+      TEST_VERIFY_EXIT (servers[i].transport != NULL);
+      servers[i].address = (struct sockaddr_in)
+        {
+          .sin_family = AF_INET,
+          .sin_addr.s_addr = htonl (INADDR_LOOPBACK),
+          .sin_port = htons (servers[i].transport->xp_port),
+        };
+      servers[i].xid = 0xabcd0101 + i;
+      if (test_verbose)
+        printf ("info: setting up server %d xid=%x on port %d\n",
+                i, servers[i].xid, servers[i].transport->xp_port);
+
+      server_id = i;
+      servers[i].pid = xfork ();
+      if (servers[i].pid == 0)
+        {
+          TEST_VERIFY (svc_register (servers[i].transport,
+                                     PROGNUM, VERSNUM, server_dispatch, 0));
+          svc_run ();
+          FAIL_EXIT1 ("supposed to be unreachable");
+        }
+      /* We need to close the socket so that we do not accidentally
+         consume the request.  */
+      TEST_VERIFY (close (servers[i].transport->xp_sock) == 0);
+    }
+
+
+  /* The following code mirrors what ypbind does.  */
+
+  /* Copied from clnt_udp.c (like ypbind).  */
+  struct cu_data
+  {
+    int cu_sock;
+    bool_t cu_closeit;
+    struct sockaddr_in cu_raddr;
+    int cu_rlen;
+    struct timeval cu_wait;
+    struct timeval cu_total;
+    struct rpc_err cu_error;
+    XDR cu_outxdrs;
+    u_int cu_xdrpos;
+    u_int cu_sendsz;
+    char *cu_outbuf;
+    u_int cu_recvsz;
+    char cu_inbuf[1];
+  };
+
+  int client_socket = xsocket (AF_INET, SOCK_DGRAM | SOCK_NONBLOCK, 0);
+  CLIENT *clnt = clntudp_create (&servers[0].address, PROGNUM, VERSNUM,
+                                 /* 5 seconds per-response timeout.  */
+                                 ((struct timeval) { 5, 0 }),
+                                 &client_socket);
+  TEST_VERIFY (clnt != NULL);
+  clnt->cl_auth = authunix_create_default ();
+  {
+    struct timeval zero = { 0, 0 };
+    TEST_VERIFY (clnt_control (clnt, CLSET_TIMEOUT, (void *) &zero));
+  }
+
+  /* Poke at internal data structures (like ypbind).  */
+  struct cu_data *cu = (struct cu_data *) clnt->cl_private;
+
+  /* Send a ping to each server.  */
+  double before_pings = get_ticks ();
+  for (int i = 0; i < SERVER_COUNT; ++i)
+    {
+      if (test_verbose)
+        printf ("info: sending server %d ping\n", i);
+      /* Reset the xid because it is changed by each invocation of
+         clnt_call.  Subtract one to compensate for the xid update
+         during the call.  */
+      *((u_int32_t *) (cu->cu_outbuf)) = servers[i].xid - 1;
+      cu->cu_raddr = servers[i].address;
+
+      struct test_query query = { .a = 100, .b = i + 1 };
+      if (i == 1)
+        /* Shorter timeout to prefer this server.  These timeouts must
+           be much shorter than the 5-second per-response timeout
+           configured with clntudp_create.  */
+        query.timeout_ms = 700;
+      else
+        query.timeout_ms = 1400;
+      struct test_response response = { 0 };
+      /* NB: Do not check the return value.  The server reply will
+         prove that the call worked.  */
+      double before_one_ping = get_ticks ();
+      clnt_call (clnt, PROC_ADD,
+                 xdr_test_query, (void *) &query,
+                 xdr_test_response, (void *) &response,
+                 ((struct timeval) { 0, 0 }));
+      double after_one_ping = get_ticks ();
+      if (test_verbose)
+        printf ("info: non-blocking send took %f seconds\n",
+                after_one_ping - before_one_ping);
+      /* clnt_call should return immediately.  Accept some delay in
+         case the process is descheduled.  */
+      TEST_VERIFY (after_one_ping - before_one_ping < 0.3);
+    }
+
+  /* Collect the non-blocking response.  */
+  if (test_verbose)
+    printf ("info: collecting response\n");
+  struct test_response response = { 0 };
+  TEST_VERIFY
+    (clnt_call (clnt, PROC_ADD, NULL, NULL,
+                xdr_test_response, (void *) &response,
+                ((struct timeval) { 0, 0 })) == RPC_SUCCESS);
+  double after_pings = get_ticks ();
+  if (test_verbose)
+    printf ("info: send/receive took %f seconds\n",
+            after_pings - before_pings);
+  /* Expected timeout is 0.7 seconds.  */
+  TEST_VERIFY (0.7 <= after_pings - before_pings);
+  TEST_VERIFY (after_pings - before_pings < 1.2);
+
+  uint32_t xid;
+  memcpy (&xid, &cu->cu_inbuf, sizeof (xid));
+  if (test_verbose)
+    printf ("info: non-blocking response: xid=%x server_id=%u seq=%u sum=%u\n",
+            xid, response.server_id, response.seq, response.sum);
+  /* Check that the reply from the preferred server was used.  */
+  TEST_VERIFY (servers[1].xid == xid);
+  TEST_VERIFY (response.server_id == 1);
+  TEST_VERIFY (response.seq == 1);
+  TEST_VERIFY (response.sum == 102);
+
+  auth_destroy (clnt->cl_auth);
+  clnt_destroy (clnt);
+
+  for (int i = 0; i < SERVER_COUNT; ++i)
+    {
+      if (test_verbose)
+        printf ("info: requesting server %d termination\n", i);
+      client_socket = RPC_ANYSOCK;
+      clnt = clntudp_create (&servers[i].address, PROGNUM, VERSNUM,
+                             ((struct timeval) { 5, 0 }),
+                             &client_socket);
+      TEST_VERIFY_EXIT (clnt != NULL);
+      TEST_VERIFY (clnt_call (clnt, PROC_EXIT,
+                              (xdrproc_t) xdr_void, NULL,
+                              (xdrproc_t) xdr_void, NULL,
+                              ((struct timeval) { 3, 0 })) == RPC_SUCCESS);
+      clnt_destroy (clnt);