From patchwork Tue Feb 28 14:46:26 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 733582 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3vXhH42bcRz9s7m for ; Wed, 1 Mar 2017 01:46:39 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="P2L5V6EK"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:to:subject:mime-version:content-type :content-transfer-encoding:message-id:from; q=dns; s=default; b= uXKRuEa1/xJV5kc3eGp64Ckr1uFO2VNE90FT62LOxOWuFHiUgvXUfoXU5w7mkFhQ olylFBhonEqLOFFeERbEIyoEop2CFUqqX5/jywRnrhepwPMiKUZh9iyecNlyP0n7 +8OlxZbDiSPFT01Lhw5S8N3qtwHUHam38e+bE+L7YJQ= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:to:subject:mime-version:content-type :content-transfer-encoding:message-id:from; s=default; bh=qdmPd6 H+BexDDHnSxcjZ5pMvJFQ=; b=P2L5V6EKmbAcSoPa6P5vhVPUaopNUhP7K+8TPb 7N6TFySeS+VBbPggPtfPSEyPcoZW8TWxk4eDqfXw/4kP64aCMDQ/sPLhMijim1hd IfxMQJndrYBB9VVYsJBO6PFdOi46PH8lGRpwvfezrTcI70IRpxRRsu1kmGQeGpSa oyAzY= Received: (qmail 8717 invoked by alias); 28 Feb 2017 14:46:31 -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 8649 invoked by uid 89); 28 Feb 2017 14:46:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Date: Tue, 28 Feb 2017 15:46:26 +0100 To: libc-alpha@sourceware.org Subject: [PATCH COMMITTED] sunrpc: Do not unregister services if not registered [BZ #5010] User-Agent: Heirloom mailx 12.5 7/5/10 MIME-Version: 1.0 Message-Id: <20170228144626.5B8FE4062ED01@oldenburg.str.redhat.com> From: fweimer@redhat.com (Florian Weimer) 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. 2017-02-28 Florian Weimer [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 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 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; -} - - /* 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 0000000..8934094 --- /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 + . */ + +/* 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 +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +/* 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