"source routing" in rpcbind

Fix a bug in rpcbind for multihomed hosts. If the server had interfaces on
two separate subnets, and a client on the first subnet contacted rpcbind at
the address on the second subnet, rpcbind would advertise addresses on the
first subnet. This is a bug, because it should prefer to advertise the
address where it was contacted. The requested service might be firewalled
off from the address on the first subnet, for example.

usr.sbin/rpcbind/check_bound.c
	If the address on which a request was received is known, pass that
	to addrmerge as the clnt_uaddr parameter. That is what addrmerge's
	comment indicates the parameter is supposed to mean. The previous
	behavior is that clnt_uaddr would contain the address from which the
	client sent the request.

usr.sbin/rpcbind/util.c
	Modify addrmerge to prefer to use an IP that is equal to clnt_uaddr,
	if one is found. Refactor the relevant portion of the function for
	clarity, and to reduce the number of ifdefs.

etc/mtree/BSD.tests.dist
usr.sbin/rpcbind/tests/Makefile
usr.sbin/rpcbind/tests/addrmerge_test.c
	Add unit tests for usr.sbin/rpcbind/util.c:addrmerge.

usr.sbin/rpcbind/check_bound.c
usr.sbin/rpcbind/rpcbind.h
usr.sbin/rpcbind/util.c
	Constify some function arguments

Reviewed by:	imp
MFC after:	4 weeks
Sponsored by:	Spectra Logic Corp
Differential Revision:	https://reviews.freebsd.org/D4690

7 files changed, 981 insertions, 49 deletions

diff --git a/etc/mtree/BSD.tests.dist b/etc/mtree/BSD.tests.dist
index ad76402..ff32324 100644
--- a/etc/mtree/BSD.tests.dist
+++ b/etc/mtree/BSD.tests.dist
@@ -622,6 +622,8 @@
         ..
         pw
         ..
+        rpcbind
+        ..
         sa
         ..
     ..
diff --git a/usr.sbin/rpcbind/Makefile b/usr.sbin/rpcbind/Makefile
index 2b679f4..b328260 100644
--- a/usr.sbin/rpcbind/Makefile
+++ b/usr.sbin/rpcbind/Makefile
@@ -14,6 +14,10 @@ CFLAGS+= -DPORTMAP -DLIBWRAP
 CFLAGS+= -DINET6
 .endif
 
+.if ${MK_TESTS} != "no"
+SUBDIR+=	tests
+.endif
+
 WARNS?=	1
 
 LIBADD=	wrap
diff --git a/usr.sbin/rpcbind/check_bound.c b/usr.sbin/rpcbind/check_bound.c
index 3691f5f..64b73c7 100644
--- a/usr.sbin/rpcbind/check_bound.c
+++ b/usr.sbin/rpcbind/check_bound.c
@@ -50,6 +50,7 @@ static	char sccsid[] = "@(#)check_bound.c 1.11 89/04/21 Copyr 1989 Sun Micro";
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <rpc/rpc.h>
+#include <rpc/svc_dg.h>
 #include <stdio.h>
 #include <netconfig.h>
 #include <syslog.h>
@@ -159,6 +160,7 @@ char *
 mergeaddr(SVCXPRT *xprt, char *netid, char *uaddr, char *saddr)
 {
 	struct fdlist *fdl;
+	struct svc_dg_data *dg_data;
 	char *c_uaddr, *s_uaddr, *m_uaddr, *allocated_uaddr = NULL;
 
 	for (fdl = fdhead; fdl; fdl = fdl->next)
@@ -170,11 +172,20 @@ mergeaddr(SVCXPRT *xprt, char *netid, char *uaddr, char *saddr)
 		/* that server died */
 		return (nullstring);
 	/*
+	 * Try to determine the local address on which the client contacted us,
+	 * so we can send a reply from the same address.  If it's unknown, then
+	 * try to determine which address the client used, and pick a nearby
+	 * local address.
+	 *
 	 * If saddr is not NULL, the remote client may have included the
 	 * address by which it contacted us.  Use that for the "client" uaddr,
 	 * otherwise use the info from the SVCXPRT.
 	 */
-	if (saddr != NULL) {
+	dg_data = (struct svc_dg_data*)xprt->xp_p2;
+	if (dg_data != NULL && dg_data->su_srcaddr.buf != NULL) {
+		c_uaddr = taddr2uaddr(fdl->nconf, &dg_data->su_srcaddr);
+	}
+	else if (saddr != NULL) {
 		c_uaddr = saddr;
 	} else {
 		c_uaddr = taddr2uaddr(fdl->nconf, svc_getrpccaller(xprt));
@@ -217,7 +228,7 @@ mergeaddr(SVCXPRT *xprt, char *netid, char *uaddr, char *saddr)
  * structure should not be freed.
  */
 struct netconfig *
-rpcbind_get_conf(char *netid)
+rpcbind_get_conf(const char *netid)
 {
 	struct fdlist *fdl;
 
diff --git a/usr.sbin/rpcbind/rpcbind.h b/usr.sbin/rpcbind/rpcbind.h
index 4aba420..309bc0b 100644
--- a/usr.sbin/rpcbind/rpcbind.h
+++ b/usr.sbin/rpcbind/rpcbind.h
@@ -85,7 +85,7 @@ extern char *tcp_uaddr;		/* Universal TCP address */
 int add_bndlist(struct netconfig *, struct netbuf *);
 bool_t is_bound(char *, char *);
 char *mergeaddr(SVCXPRT *, char *, char *, char *);
-struct netconfig *rpcbind_get_conf(char *);
+struct netconfig *rpcbind_get_conf(const char *);
 
 void rpcbs_init(void); 
 void rpcbs_procinfo(rpcvers_t, rpcproc_t);
@@ -134,8 +134,8 @@ extern void pmap_service(struct svc_req *, SVCXPRT *);
 void write_warmstart(void);
 void read_warmstart(void);
 
-char *addrmerge(struct netbuf *caller, char *serv_uaddr, char *clnt_uaddr,
-		     char *netid);
+char *addrmerge(struct netbuf *caller, const char *serv_uaddr,
+		const char *clnt_uaddr, char const *netid);
 int listen_addr(const struct sockaddr *sa);
 void network_init(void);
 struct sockaddr *local_sa(int);
diff --git a/usr.sbin/rpcbind/tests/Makefile b/usr.sbin/rpcbind/tests/Makefile
new file mode 100644
index 0000000..4b0cf15
--- /dev/null
+++ b/usr.sbin/rpcbind/tests/Makefile
@@ -0,0 +1,17 @@
+# $FreeBSD$
+
+.include <src.opts.mk>
+
+.PATH:	${.CURDIR}/..
+
+ATF_TESTS_C=	addrmerge_test
+CFLAGS+=	-I${.CURDIR}/.. -Wno-cast-qual
+SRCS.addrmerge_test=	addrmerge_test.c util.c
+
+.if ${MK_INET6_SUPPORT} != "no"
+CFLAGS+= -DINET6
+.endif
+
+WARNS?=	3
+
+.include <bsd.test.mk>
diff --git a/usr.sbin/rpcbind/tests/addrmerge_test.c b/usr.sbin/rpcbind/tests/addrmerge_test.c
new file mode 100644
index 0000000..357354a
--- /dev/null
+++ b/usr.sbin/rpcbind/tests/addrmerge_test.c
@@ -0,0 +1,849 @@
+/*-
+ * Copyright (c) 2014 Spectra Logic Corporation
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions, and the following disclaimer,
+ *    without modification.
+ * 2. Redistributions in binary form must reproduce at minimum a disclaimer
+ *    substantially similar to the "NO WARRANTY" disclaimer below
+ *    ("Disclaimer") and any redistribution must be conditioned upon
+ *    including a substantially similar Disclaimer requirement for further
+ *    binary redistribution.
+ *
+ * NO WARRANTY
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * HOLDERS OR CONTRIBUTORS BE LIABLE FOR SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
+ * IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGES.
+ *
+ * $FreeBSD$
+ */
+
+#include <rpc/rpc.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+
+#include <net/if.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
+
+#include <ifaddrs.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+#include <atf-c.h>
+
+#include "rpcbind.h"
+
+#define MAX_IFADDRS 16
+
+int debugging = false;
+
+/* Data for mocking getifaddrs */
+struct ifaddr_storage {
+	struct ifaddrs ifaddr;
+	struct sockaddr_storage addr;
+	struct sockaddr_storage mask;
+	struct sockaddr_storage bcast;
+} mock_ifaddr_storage[MAX_IFADDRS];
+struct ifaddrs *mock_ifaddrs = NULL;
+int ifaddr_count = 0; 
+
+/* Data for mocking listen_addr */
+int bind_address_count = 0;
+struct sockaddr* bind_addresses[MAX_IFADDRS];
+
+/* Stub library functions */
+void
+freeifaddrs(struct ifaddrs *ifp __unused)
+{
+	return ;
+}
+
+int
+getifaddrs(struct ifaddrs **ifap)
+{
+	*ifap = mock_ifaddrs;
+	return (0);
+}
+
+static void
+mock_ifaddr4(const char* name, const char* addr, const char* mask,
+    const char* bcast, unsigned int flags, bool bind)
+{
+	struct ifaddrs *ifaddr = &mock_ifaddr_storage[ifaddr_count].ifaddr;
+	struct sockaddr_in *in = (struct sockaddr_in*)
+	    			&mock_ifaddr_storage[ifaddr_count].addr;
+	struct sockaddr_in *mask_in = (struct sockaddr_in*)
+	    			&mock_ifaddr_storage[ifaddr_count].mask;
+	struct sockaddr_in *bcast_in = (struct sockaddr_in*)
+	    			&mock_ifaddr_storage[ifaddr_count].bcast;
+
+	in->sin_family = AF_INET;
+	in->sin_port = 0;
+	in->sin_len = sizeof(in);
+	in->sin_addr.s_addr = inet_addr(addr);
+	mask_in->sin_family = AF_INET;
+	mask_in->sin_port = 0;
+	mask_in->sin_len = sizeof(mask_in);
+	mask_in->sin_addr.s_addr = inet_addr(mask);
+	bcast_in->sin_family = AF_INET;
+	bcast_in->sin_port = 0;
+	bcast_in->sin_len = sizeof(bcast_in);
+	bcast_in->sin_addr.s_addr = inet_addr(bcast);
+	*ifaddr = (struct ifaddrs) {
+		.ifa_next = NULL,
+		.ifa_name = (char*) name,
+		.ifa_flags = flags,
+		.ifa_addr = (struct sockaddr*) in,
+		.ifa_netmask = (struct sockaddr*) mask_in,
+		.ifa_broadaddr = (struct sockaddr*) bcast_in,
+		.ifa_data = NULL,	/* addrmerge doesn't care*/
+	};
+
+	if (ifaddr_count > 0)
+		mock_ifaddr_storage[ifaddr_count - 1].ifaddr.ifa_next = ifaddr;
+	ifaddr_count++;
+	mock_ifaddrs = &mock_ifaddr_storage[0].ifaddr;
+
+	/* Optionally simulate binding an ip ala "rpcbind -h foo" */
+	if (bind) {
+		bind_addresses[bind_address_count] = (struct sockaddr*)in;
+		bind_address_count++;
+	}
+}
+
+#ifdef INET6
+static void
+mock_ifaddr6(const char* name, const char* addr, const char* mask,
+    const char* bcast, unsigned int flags, uint32_t scope_id, bool bind)
+{
+	struct ifaddrs *ifaddr = &mock_ifaddr_storage[ifaddr_count].ifaddr;
+	struct sockaddr_in6 *in6 = (struct sockaddr_in6*)
+	    			&mock_ifaddr_storage[ifaddr_count].addr;
+	struct sockaddr_in6 *mask_in6 = (struct sockaddr_in6*)
+	    			&mock_ifaddr_storage[ifaddr_count].mask;
+	struct sockaddr_in6 *bcast_in6 = (struct sockaddr_in6*)
+	    			&mock_ifaddr_storage[ifaddr_count].bcast;
+
+	in6->sin6_family = AF_INET6;
+	in6->sin6_port = 0;
+	in6->sin6_len = sizeof(*in6);
+	in6->sin6_scope_id = scope_id;
+	ATF_REQUIRE_EQ(1, inet_pton(AF_INET6, addr, (void*)&in6->sin6_addr));
+	mask_in6->sin6_family = AF_INET6;
+	mask_in6->sin6_port = 0;
+	mask_in6->sin6_len = sizeof(*mask_in6);
+	mask_in6->sin6_scope_id = scope_id;
+	ATF_REQUIRE_EQ(1, inet_pton(AF_INET6, mask,
+	    (void*)&mask_in6->sin6_addr));
+	bcast_in6->sin6_family = AF_INET6;
+	bcast_in6->sin6_port = 0;
+	bcast_in6->sin6_len = sizeof(*bcast_in6);
+	bcast_in6->sin6_scope_id = scope_id;
+	ATF_REQUIRE_EQ(1, inet_pton(AF_INET6, bcast,
+	    (void*)&bcast_in6->sin6_addr));
+	*ifaddr = (struct ifaddrs) {
+		.ifa_next = NULL,
+		.ifa_name = (char*) name,
+		.ifa_flags = flags,
+		.ifa_addr = (struct sockaddr*) in6,
+		.ifa_netmask = (struct sockaddr*) mask_in6,
+		.ifa_broadaddr = (struct sockaddr*) bcast_in6,
+		.ifa_data = NULL,	/* addrmerge doesn't care*/
+	};
+
+	if (ifaddr_count > 0)
+		mock_ifaddr_storage[ifaddr_count - 1].ifaddr.ifa_next = ifaddr;
+	ifaddr_count++;
+	mock_ifaddrs = &mock_ifaddr_storage[0].ifaddr;
+
+	/* Optionally simulate binding an ip ala "rpcbind -h foo" */
+	if (bind) {
+		bind_addresses[bind_address_count] = (struct sockaddr*)in6;
+		bind_address_count++;
+	}
+}
+#else
+static void
+mock_ifaddr6(const char* name __unused, const char* addr __unused,
+    const char* mask __unused, const char* bcast __unused,
+    unsigned int flags __unused, uint32_t scope_id __unused, bool bind __unused)
+{
+}
+#endif /*INET6 */
+
+static void
+mock_lo0(void)
+{
+	/* 
+	 * This broadcast address looks wrong, but it's what getifaddrs(2)
+	 * actually returns.  It's invalid because IFF_BROADCAST is not set
+	 */
+	mock_ifaddr4("lo0", "127.0.0.1", "255.0.0.0", "127.0.0.1",
+	    IFF_LOOPBACK | IFF_UP | IFF_RUNNING | IFF_MULTICAST, false);
+	mock_ifaddr6("lo0", "::1", "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff",
+	    "::1",
+	    IFF_LOOPBACK | IFF_UP | IFF_RUNNING | IFF_MULTICAST, 0, false);
+}
+
+static void
+mock_igb0(void)
+{
+	mock_ifaddr4("igb0", "192.0.2.2", "255.255.255.128", "192.0.2.127",
+	    IFF_UP | IFF_BROADCAST | IFF_RUNNING | IFF_SIMPLEX | IFF_MULTICAST,
+	    false);
+	mock_ifaddr6("igb0", "2001:db8::2", "ffff:ffff:ffff:ffff::",
+	    "2001:db8::ffff:ffff:ffff:ffff",
+	    IFF_UP | IFF_BROADCAST | IFF_RUNNING | IFF_SIMPLEX | IFF_MULTICAST,
+	    0, false);
+	/* Link local address */
+	mock_ifaddr6("igb0", "fe80::2", "ffff:ffff:ffff:ffff::",
+	    "fe80::ffff:ffff:ffff:ffff",
+	    IFF_UP | IFF_BROADCAST | IFF_RUNNING | IFF_SIMPLEX | IFF_MULTICAST,
+	    2, false);
+}
+
+/* On the same subnet as igb0 */
+static void
+mock_igb1(bool bind)
+{
+	mock_ifaddr4("igb1", "192.0.2.3", "255.255.255.128", "192.0.2.127",
+	    IFF_UP | IFF_BROADCAST | IFF_RUNNING | IFF_SIMPLEX | IFF_MULTICAST,
+	    bind);
+	mock_ifaddr6("igb1", "2001:db8::3", "ffff:ffff:ffff:ffff::",
+	    "2001:db8::ffff:ffff:ffff:ffff",
+	    IFF_UP | IFF_BROADCAST | IFF_RUNNING | IFF_SIMPLEX | IFF_MULTICAST,
+	    0, bind);
+	/* Link local address */
+	mock_ifaddr6("igb1", "fe80::3", "ffff:ffff:ffff:ffff::",
+	    "fe80::ffff:ffff:ffff:ffff",
+	    IFF_UP | IFF_BROADCAST | IFF_RUNNING | IFF_SIMPLEX | IFF_MULTICAST,
+	    3, bind);
+}
+
+/* igb2 is on a different subnet than igb0 */
+static void
+mock_igb2(void)
+{
+	mock_ifaddr4("igb2", "192.0.2.130", "255.255.255.128", "192.0.2.255",
+	    IFF_UP | IFF_BROADCAST | IFF_RUNNING | IFF_SIMPLEX | IFF_MULTICAST,
+	    false);
+	mock_ifaddr6("igb2", "2001:db8:1::2", "ffff:ffff:ffff:ffff::",
+	    "2001:db8:1:0:ffff:ffff:ffff:ffff",
+	    IFF_UP | IFF_BROADCAST | IFF_RUNNING | IFF_SIMPLEX | IFF_MULTICAST,
+	    0, false);
+}
+
+/* tun0 is a P2P interface */
+static void
+mock_tun0(void)
+{
+	mock_ifaddr4("tun0", "192.0.2.5", "255.255.255.255", "192.0.2.6",
+	    IFF_UP | IFF_RUNNING | IFF_POINTOPOINT | IFF_MULTICAST, false);
+	mock_ifaddr6("tun0", "2001:db8::5",
+	    "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff",
+	    "2001:db8::6",
+	    IFF_UP | IFF_RUNNING | IFF_POINTOPOINT | IFF_MULTICAST, 0, false);
+}
+
+
+/* Stub rpcbind functions */
+int
+listen_addr(const struct sockaddr *sa)
+{
+	int i;
+	
+	if (bind_address_count == 0)
+		return (1);
+	
+	for (i = 0; i < bind_address_count; i++) {
+		if (bind_addresses[i]->sa_family != sa->sa_family)
+			continue;
+
+		if (0 == memcmp(bind_addresses[i]->sa_data, sa->sa_data,
+		    sa->sa_len))
+			return (1);
+	}
+	return (0);
+}
+
+struct netconfig*
+rpcbind_get_conf(const char* netid __unused)
+{
+	/* Use static variables so we can return pointers to them */
+	static char* lookups = NULL;
+	static struct netconfig nconf_udp;
+#ifdef INET6
+	static struct netconfig nconf_udp6;
+#endif /* INET6 */
+
+	nconf_udp.nc_netid = "udp"; //netid_storage;
+	nconf_udp.nc_semantics = NC_TPI_CLTS;
+	nconf_udp.nc_flag = NC_VISIBLE;
+	nconf_udp.nc_protofmly = (char*)"inet";
+	nconf_udp.nc_proto = (char*)"udp";
+	nconf_udp.nc_device = (char*)"-";
+	nconf_udp.nc_nlookups = 0;
+	nconf_udp.nc_lookups = &lookups;
+
+#ifdef INET6
+	nconf_udp6.nc_netid = "udp6"; //netid_storage;
+	nconf_udp6.nc_semantics = NC_TPI_CLTS;
+	nconf_udp6.nc_flag = NC_VISIBLE;
+	nconf_udp6.nc_protofmly = (char*)"inet6";
+	nconf_udp6.nc_proto = (char*)"udp6";
+	nconf_udp6.nc_device = (char*)"-";
+	nconf_udp6.nc_nlookups = 0;
+	nconf_udp6.nc_lookups = &lookups;
+#endif /* INET6 */
+
+	if (0 == strncmp("udp", netid, sizeof("udp")))
+		return (&nconf_udp);
+#ifdef INET6
+	else if (0 == strncmp("udp6", netid, sizeof("udp6")))
+		return (&nconf_udp6);
+#endif /* INET6 */
+	else
+		return (NULL);
+}
+
+/*
+ * Helper function used by most test cases
+ * param recvdstaddr	If non-null, the uaddr on which the request was received
+ */
+static char*
+do_addrmerge4(const char* recvdstaddr)
+{
+	struct netbuf caller;
+	struct sockaddr_in caller_in;
+	const char *serv_uaddr, *clnt_uaddr, *netid;
+	
+	/* caller contains the client's IP address */
+	caller.maxlen = sizeof(struct sockaddr_storage);
+	caller.len = sizeof(caller_in);
+	caller_in.sin_family = AF_INET;
+	caller_in.sin_len = sizeof(caller_in);
+	caller_in.sin_port = 1234;
+	caller_in.sin_addr.s_addr = inet_addr("192.0.2.1");
+	caller.buf = (void*)&caller_in;
+	if (recvdstaddr != NULL)
+		clnt_uaddr = recvdstaddr;
+	else
+		clnt_uaddr = "192.0.2.1.3.46";
+
+	/* assume server is bound in INADDR_ANY port 814 */
+	serv_uaddr = "0.0.0.0.3.46";
+
+	netid = "udp";
+	return (addrmerge(&caller, serv_uaddr, clnt_uaddr, netid));
+}
+
+#ifdef INET6
+/*
+ * Variant of do_addrmerge4 where the caller has an IPv6 address
+ * param recvdstaddr	If non-null, the uaddr on which the request was received
+ */
+static char*
+do_addrmerge6(const char* recvdstaddr)
+{
+	struct netbuf caller;
+	struct sockaddr_in6 caller_in6;
+	const char *serv_uaddr, *clnt_uaddr, *netid;
+	
+	/* caller contains the client's IP address */
+	caller.maxlen = sizeof(struct sockaddr_storage);
+	caller.len = sizeof(caller_in6);
+	caller_in6.sin6_family = AF_INET6;
+	caller_in6.sin6_len = sizeof(caller_in6);
+	caller_in6.sin6_port = 1234;
+	ATF_REQUIRE_EQ(1, inet_pton(AF_INET6, "2001:db8::1",
+	    (void*)&caller_in6.sin6_addr));
+	caller.buf = (void*)&caller_in6;
+	if (recvdstaddr != NULL)
+		clnt_uaddr = recvdstaddr;
+	else
+		clnt_uaddr = "2001:db8::1.3.46";
+
+	/* assume server is bound in INADDR_ANY port 814 */
+	serv_uaddr = "::1.3.46";
+
+	netid = "udp6";
+	return (addrmerge(&caller, serv_uaddr, clnt_uaddr, netid));
+}
+
+/* Variant of do_addrmerge6 where the caller uses a link local address */
+static char*
+do_addrmerge6_ll(void)
+{
+	struct netbuf caller;
+	struct sockaddr_in6 caller_in6;
+	const char *serv_uaddr, *clnt_uaddr, *netid;
+	
+	/* caller contains the client's IP address */
+	caller.maxlen = sizeof(struct sockaddr_storage);
+	caller.len = sizeof(caller_in6);
+	caller_in6.sin6_family = AF_INET6;
+	caller_in6.sin6_len = sizeof(caller_in6);
+	caller_in6.sin6_port = 1234;
+	caller_in6.sin6_scope_id = 2; /* same as igb0 */
+	ATF_REQUIRE_EQ(1, inet_pton(AF_INET6, "fe80::beef",
+	    (void*)&caller_in6.sin6_addr));
+	caller.buf = (void*)&caller_in6;
+	clnt_uaddr = "fe80::beef.3.46";
+
+	/* assume server is bound in INADDR_ANY port 814 */
+	serv_uaddr = "::1.3.46";
+
+	netid = "udp6";
+	return (addrmerge(&caller, serv_uaddr, clnt_uaddr, netid));
+}
+#endif /* INET6 */
+
+ATF_TC_WITHOUT_HEAD(addrmerge_noifaddrs);
+ATF_TC_BODY(addrmerge_noifaddrs, tc)
+{
+	char* maddr;
+
+	maddr = do_addrmerge4(NULL);
+
+	/* Since getifaddrs returns null, addrmerge must too */
+	ATF_CHECK_EQ(NULL, maddr);
+}
+
+ATF_TC_WITHOUT_HEAD(addrmerge_localhost_only);
+ATF_TC_BODY(addrmerge_localhost_only, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return localhost only */
+	mock_lo0();
+
+	maddr = do_addrmerge4(NULL);
+
+	/* We must return localhost if there is nothing better */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("127.0.0.1.3.46", maddr);
+}
+
+ATF_TC_WITHOUT_HEAD(addrmerge_singlehomed);
+ATF_TC_BODY(addrmerge_singlehomed, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return one public address */
+	mock_lo0();
+	mock_igb0();
+
+	maddr = do_addrmerge4(NULL);
+
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("192.0.2.2.3.46", maddr);
+}
+
+ATF_TC_WITHOUT_HEAD(addrmerge_one_addr_on_each_subnet);
+ATF_TC_BODY(addrmerge_one_addr_on_each_subnet, tc)
+{
+	char *maddr;
+	
+	mock_lo0();
+	mock_igb0();
+	mock_igb2();
+
+	maddr = do_addrmerge4(NULL);
+
+	/* We must return the address on the caller's subnet */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("192.0.2.2.3.46", maddr);
+}
+
+
+/*
+ * Like addrmerge_one_addr_on_each_subnet, but getifaddrs returns a different
+ * order
+ */
+ATF_TC_WITHOUT_HEAD(addrmerge_one_addr_on_each_subnet_rev);
+ATF_TC_BODY(addrmerge_one_addr_on_each_subnet_rev, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return one public address on each of two subnets */
+	mock_igb2();
+	mock_igb0();
+	mock_lo0();
+
+	maddr = do_addrmerge4(NULL);
+
+	/* We must return the address on the caller's subnet */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("192.0.2.2.3.46", maddr);
+}
+
+ATF_TC_WITHOUT_HEAD(addrmerge_point2point);
+ATF_TC_BODY(addrmerge_point2point, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return one normal and one p2p address */
+	mock_lo0();
+	mock_igb2();
+	mock_tun0();
+
+	maddr = do_addrmerge4(NULL);
+
+	/* addrmerge should disprefer P2P interfaces */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("192.0.2.130.3.46", maddr);
+}
+
+/* Like addrerge_point2point, but getifaddrs returns a different order */
+ATF_TC_WITHOUT_HEAD(addrmerge_point2point_rev);
+ATF_TC_BODY(addrmerge_point2point_rev, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return one normal and one p2p address */
+	mock_tun0();
+	mock_igb2();
+	mock_lo0();
+
+	maddr = do_addrmerge4(NULL);
+
+	/* addrmerge should disprefer P2P interfaces */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("192.0.2.130.3.46", maddr);
+}
+
+/*
+ * Simulate using rpcbind -h to select just one ip when the subnet has
+ * multiple
+ */
+ATF_TC_WITHOUT_HEAD(addrmerge_bindip);
+ATF_TC_BODY(addrmerge_bindip, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return one public address on each of two subnets */
+	mock_lo0();
+	mock_igb0();
+	mock_igb1(true);
+
+	maddr = do_addrmerge4(NULL);
+
+	/* We must return the address to which we are bound */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("192.0.2.3.3.46", maddr);
+}
+
+/* Like addrmerge_bindip, but getifaddrs returns a different order */
+ATF_TC_WITHOUT_HEAD(addrmerge_bindip_rev);
+ATF_TC_BODY(addrmerge_bindip_rev, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return one public address on each of two subnets */
+	mock_igb1(true);
+	mock_igb0();
+	mock_lo0();
+
+	maddr = do_addrmerge4(NULL);
+
+	/* We must return the address to which we are bound */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("192.0.2.3.3.46", maddr);
+}
+
+/* 
+ * The address on which the request was received is known, and is provided as
+ * the hint.
+ */
+ATF_TC_WITHOUT_HEAD(addrmerge_recvdstaddr);
+ATF_TC_BODY(addrmerge_recvdstaddr, tc)
+{
+	char *maddr;
+	
+	mock_lo0();
+	mock_igb0();
+	mock_igb1(false);
+
+	maddr = do_addrmerge4("192.0.2.2.3.46");
+
+	/* We must return the address on which the request was received */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("192.0.2.2.3.46", maddr);
+}
+
+ATF_TC_WITHOUT_HEAD(addrmerge_recvdstaddr_rev);
+ATF_TC_BODY(addrmerge_recvdstaddr_rev, tc)
+{
+	char *maddr;
+	
+	mock_igb1(false);
+	mock_igb0();
+	mock_lo0();
+
+	maddr = do_addrmerge4("192.0.2.2.3.46");
+
+	/* We must return the address on which the request was received */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("192.0.2.2.3.46", maddr);
+}
+
+#ifdef INET6
+ATF_TC_WITHOUT_HEAD(addrmerge_localhost_only6);
+ATF_TC_BODY(addrmerge_localhost_only6, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return localhost only */
+	mock_lo0();
+
+	maddr = do_addrmerge6(NULL);
+
+	/* We must return localhost if there is nothing better */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("::1.3.46", maddr);
+}
+
+ATF_TC_WITHOUT_HEAD(addrmerge_singlehomed6);
+ATF_TC_BODY(addrmerge_singlehomed6, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return one public address */
+	mock_lo0();
+	mock_igb0();
+
+	maddr = do_addrmerge6(NULL);
+
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("2001:db8::2.3.46", maddr);
+}
+
+ATF_TC_WITHOUT_HEAD(addrmerge_one_addr_on_each_subnet6);
+ATF_TC_BODY(addrmerge_one_addr_on_each_subnet6, tc)
+{
+	char *maddr;
+	
+	mock_lo0();
+	mock_igb0();
+	mock_igb2();
+
+	maddr = do_addrmerge6(NULL);
+
+	/* We must return the address on the caller's subnet */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("2001:db8::2.3.46", maddr);
+}
+
+
+/*
+ * Like addrmerge_one_addr_on_each_subnet6, but getifaddrs returns a different
+ * order
+ */
+ATF_TC_WITHOUT_HEAD(addrmerge_one_addr_on_each_subnet6_rev);
+ATF_TC_BODY(addrmerge_one_addr_on_each_subnet6_rev, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return one public address on each of two subnets */
+	mock_igb2();
+	mock_igb0();
+	mock_lo0();
+
+	maddr = do_addrmerge6(NULL);
+
+	/* We must return the address on the caller's subnet */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("2001:db8::2.3.46", maddr);
+}
+
+ATF_TC_WITHOUT_HEAD(addrmerge_point2point6);
+ATF_TC_BODY(addrmerge_point2point6, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return one normal and one p2p address */
+	mock_lo0();
+	mock_igb2();
+	mock_tun0();
+
+	maddr = do_addrmerge6(NULL);
+
+	/* addrmerge should disprefer P2P interfaces */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("2001:db8:1::2.3.46", maddr);
+}
+
+/* Like addrerge_point2point, but getifaddrs returns a different order */
+ATF_TC_WITHOUT_HEAD(addrmerge_point2point6_rev);
+ATF_TC_BODY(addrmerge_point2point6_rev, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return one normal and one p2p address */
+	mock_tun0();
+	mock_igb2();
+	mock_lo0();
+
+	maddr = do_addrmerge6(NULL);
+
+	/* addrmerge should disprefer P2P interfaces */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("2001:db8:1::2.3.46", maddr);
+}
+
+ATF_TC_WITHOUT_HEAD(addrmerge_bindip6);
+ATF_TC_BODY(addrmerge_bindip6, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return one public address on each of two subnets */
+	mock_lo0();
+	mock_igb0();
+	mock_igb1(true);
+
+	maddr = do_addrmerge6(NULL);
+
+	/* We must return the address to which we are bound */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("2001:db8::3.3.46", maddr);
+}
+
+/* Like addrerge_bindip, but getifaddrs returns a different order */
+ATF_TC_WITHOUT_HEAD(addrmerge_bindip6_rev);
+ATF_TC_BODY(addrmerge_bindip6_rev, tc)
+{
+	char *maddr;
+	
+	/* getifaddrs will return one public address on each of two subnets */
+	mock_igb1(true);
+	mock_igb0();
+	mock_lo0();
+
+	maddr = do_addrmerge6(NULL);
+
+	/* We must return the address to which we are bound */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("2001:db8::3.3.46", maddr);
+}
+
+/* 
+ * IPv6 Link Local addresses with the same scope id as the caller, if the caller
+ * is also a link local address, should be preferred
+ */
+ATF_TC_WITHOUT_HEAD(addrmerge_ipv6_linklocal);
+ATF_TC_BODY(addrmerge_ipv6_linklocal, tc)
+{
+	char *maddr;
+	
+	/* 
+	 * getifaddrs will return two link local addresses with the same netmask
+	 * and prefix but different scope IDs
+	 */
+	mock_igb1(false);
+	mock_igb0();
+	mock_lo0();
+
+	maddr = do_addrmerge6_ll();
+
+	/* We must return the address to which we are bound */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("fe80::2.3.46", maddr);
+}
+
+ATF_TC_WITHOUT_HEAD(addrmerge_ipv6_linklocal_rev);
+ATF_TC_BODY(addrmerge_ipv6_linklocal_rev, tc)
+{
+	char *maddr;
+	
+	/* 
+	 * getifaddrs will return two link local addresses with the same netmask
+	 * and prefix but different scope IDs
+	 */
+	mock_lo0();
+	mock_igb0();
+	mock_igb1(false);
+
+	maddr = do_addrmerge6_ll();
+
+	/* We must return the address to which we are bound */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("fe80::2.3.46", maddr);
+}
+
+ATF_TC_WITHOUT_HEAD(addrmerge_recvdstaddr6);
+ATF_TC_BODY(addrmerge_recvdstaddr6, tc)
+{
+	char *maddr;
+	
+	mock_lo0();
+	mock_igb0();
+	mock_igb1(false);
+
+	maddr = do_addrmerge6("2001:db8::2.3.46");
+
+	/* We must return the address on which the request was received */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("2001:db8::2.3.46", maddr);
+}
+
+ATF_TC_WITHOUT_HEAD(addrmerge_recvdstaddr6_rev);
+ATF_TC_BODY(addrmerge_recvdstaddr6_rev, tc)
+{
+	char *maddr;
+	
+	mock_igb1(false);
+	mock_igb0();
+	mock_lo0();
+
+	maddr = do_addrmerge6("2001:db8::2.3.46");
+
+	/* We must return the address on which the request was received */
+	ATF_REQUIRE(maddr != NULL);
+	ATF_CHECK_STREQ("2001:db8::2.3.46", maddr);
+}
+#endif /* INET6 */
+
+
+ATF_TP_ADD_TCS(tp)
+{
+	ATF_TP_ADD_TC(tp, addrmerge_noifaddrs);
+	ATF_TP_ADD_TC(tp, addrmerge_localhost_only);
+	ATF_TP_ADD_TC(tp, addrmerge_singlehomed);
+	ATF_TP_ADD_TC(tp, addrmerge_one_addr_on_each_subnet);
+	ATF_TP_ADD_TC(tp, addrmerge_one_addr_on_each_subnet_rev);
+	ATF_TP_ADD_TC(tp, addrmerge_point2point);
+	ATF_TP_ADD_TC(tp, addrmerge_point2point_rev);
+	ATF_TP_ADD_TC(tp, addrmerge_bindip);
+	ATF_TP_ADD_TC(tp, addrmerge_bindip_rev);
+	ATF_TP_ADD_TC(tp, addrmerge_recvdstaddr);
+	ATF_TP_ADD_TC(tp, addrmerge_recvdstaddr_rev);
+#ifdef INET6
+	ATF_TP_ADD_TC(tp, addrmerge_localhost_only6);
+	ATF_TP_ADD_TC(tp, addrmerge_singlehomed6);
+	ATF_TP_ADD_TC(tp, addrmerge_one_addr_on_each_subnet6);
+	ATF_TP_ADD_TC(tp, addrmerge_one_addr_on_each_subnet6_rev);
+	ATF_TP_ADD_TC(tp, addrmerge_point2point6);
+	ATF_TP_ADD_TC(tp, addrmerge_point2point6_rev);
+	ATF_TP_ADD_TC(tp, addrmerge_bindip6);
+	ATF_TP_ADD_TC(tp, addrmerge_bindip6_rev);
+	ATF_TP_ADD_TC(tp, addrmerge_ipv6_linklocal);
+	ATF_TP_ADD_TC(tp, addrmerge_ipv6_linklocal_rev);
+	ATF_TP_ADD_TC(tp, addrmerge_recvdstaddr6);
+	ATF_TP_ADD_TC(tp, addrmerge_recvdstaddr6_rev);
+#endif
+
+	return (atf_no_error());
+}
diff --git a/usr.sbin/rpcbind/util.c b/usr.sbin/rpcbind/util.c
index 8ddb13e..da6a5ab 100644
--- a/usr.sbin/rpcbind/util.c
+++ b/usr.sbin/rpcbind/util.c
@@ -56,7 +56,7 @@ static struct sockaddr_in *local_in4;
 static struct sockaddr_in6 *local_in6;
 #endif
 
-static int bitmaskcmp(void *, void *, void *, int);
+static int bitmaskcmp(struct sockaddr *, struct sockaddr *, struct sockaddr *);
 
 /*
  * For all bits set in "mask", compare the corresponding bits in
@@ -64,10 +64,34 @@ static int bitmaskcmp(void *, void *, void *, int);
  * match.
  */
 static int
-bitmaskcmp(void *dst, void *src, void *mask, int bytelen)
+bitmaskcmp(struct sockaddr *dst, struct sockaddr *src, struct sockaddr *mask)
 {
 	int i;
-	u_int8_t *p1 = dst, *p2 = src, *netmask = mask;
+	u_int8_t *p1, *p2, *netmask;
+	int bytelen;
+
+	if (dst->sa_family != src->sa_family ||
+	    dst->sa_family != mask->sa_family)
+		return (1);
+
+	switch (dst->sa_family) {
+	case AF_INET:
+		p1 = (uint8_t*) &SA2SINADDR(dst);
+		p2 = (uint8_t*) &SA2SINADDR(src);
+		netmask = (uint8_t*) &SA2SINADDR(mask);
+		bytelen = sizeof(struct in_addr);
+		break;
+#ifdef INET6
+	case AF_INET6:
+		p1 = (uint8_t*) &SA2SIN6ADDR(dst);
+		p2 = (uint8_t*) &SA2SIN6ADDR(src);
+		netmask = (uint8_t*) &SA2SIN6ADDR(mask);
+		bytelen = sizeof(struct in6_addr);
+		break;
+#endif
+	default:
+		return (1);
+	}
 
 	for (i = 0; i < bytelen; i++)
 		if ((p1[i] & netmask[i]) != (p2[i] & netmask[i]))
@@ -86,16 +110,18 @@ bitmaskcmp(void *dst, void *src, void *mask, int bytelen)
  * string which should be freed by the caller. On error, returns NULL.
  */
 char *
-addrmerge(struct netbuf *caller, char *serv_uaddr, char *clnt_uaddr,
-	  char *netid)
+addrmerge(struct netbuf *caller, const char *serv_uaddr, const char *clnt_uaddr,
+	  const char *netid)
 {
 	struct ifaddrs *ifap, *ifp = NULL, *bestif;
 	struct netbuf *serv_nbp = NULL, *hint_nbp = NULL, tbuf;
 	struct sockaddr *caller_sa, *hint_sa, *ifsa, *ifmasksa, *serv_sa;
 	struct sockaddr_storage ss;
 	struct netconfig *nconf;
-	char *caller_uaddr = NULL, *hint_uaddr = NULL;
+	char *caller_uaddr = NULL;
+	const char *hint_uaddr = NULL;
 	char *ret = NULL;
+	int bestif_goodness;
 
 #ifdef ND_DEBUG
 	if (debugging)
@@ -139,19 +165,29 @@ addrmerge(struct netbuf *caller, char *serv_uaddr, char *clnt_uaddr,
 		goto freeit;
 
 	/*
-	 * Loop through all interfaces. For each interface, see if it
-	 * is either the loopback interface (which we always listen
-	 * on) or is one of the addresses the program bound to (the
-	 * wildcard by default, or a subset if -h is specified) and
-	 * the network portion of its address is equal to that of the
-	 * client.  If so, we have found the interface that we want to
-	 * use.
+	 * Loop through all interface addresses.  We are listening to an address
+	 * if any of the following are true:
+	 * a) It's a loopback address
+	 * b) It was specified with the -h command line option
+	 * c) There were no -h command line options.
+	 *
+	 * Among addresses on which we are listening, choose in order of
+	 * preference an address that is:
+	 *
+	 * a) Equal to the hint
+	 * b) A link local address with the same scope ID as the client's
+	 *    address, if the client's address is also link local
+	 * c) An address on the same subnet as the client's address
+	 * d) A non-localhost, non-p2p address
+	 * e) Any usable address
 	 */
 	bestif = NULL;
+	bestif_goodness = 0;
 	for (ifap = ifp; ifap != NULL; ifap = ifap->ifa_next) {
 		ifsa = ifap->ifa_addr;
 		ifmasksa = ifap->ifa_netmask;
 
+		/* Skip addresses where we don't listen */
 		if (ifsa == NULL || ifsa->sa_family != hint_sa->sa_family ||
 		    !(ifap->ifa_flags & IFF_UP))
 			continue;
@@ -159,21 +195,29 @@ addrmerge(struct netbuf *caller, char *serv_uaddr, char *clnt_uaddr,
 		if (!(ifap->ifa_flags & IFF_LOOPBACK) && !listen_addr(ifsa))
 			continue;
 
-		switch (hint_sa->sa_family) {
-		case AF_INET:
-			/*
-			 * If the hint address matches this interface
-			 * address/netmask, then we're done.
-			 */
-			if (!bitmaskcmp(&SA2SINADDR(ifsa),
-			    &SA2SINADDR(hint_sa), &SA2SINADDR(ifmasksa),
-			    sizeof(struct in_addr))) {
-				bestif = ifap;
-				goto found;
-			}
-			break;
+		if ((hint_sa->sa_family == AF_INET) &&
+		    ((((struct sockaddr_in*)hint_sa)->sin_addr.s_addr == 
+		      ((struct sockaddr_in*)ifsa)->sin_addr.s_addr))) {
+			const int goodness = 4;
+
+			bestif_goodness = goodness;
+			bestif = ifap;
+			goto found;
+		}
 #ifdef INET6
-		case AF_INET6:
+		if ((hint_sa->sa_family == AF_INET6) &&
+		    (0 == memcmp(&((struct sockaddr_in6*)hint_sa)->sin6_addr,
+				 &((struct sockaddr_in6*)ifsa)->sin6_addr,
+				 sizeof(struct in6_addr))) &&
+		    (((struct sockaddr_in6*)hint_sa)->sin6_scope_id ==
+		    (((struct sockaddr_in6*)ifsa)->sin6_scope_id))) {
+			const int goodness = 4;
+
+			bestif_goodness = goodness;
+			bestif = ifap;
+			goto found;
+		}
+		if (hint_sa->sa_family == AF_INET6) {
 			/*
 			 * For v6 link local addresses, if the caller is on
 			 * a link-local address then use the scope id to see
@@ -184,28 +228,33 @@ addrmerge(struct netbuf *caller, char *serv_uaddr, char *clnt_uaddr,
 			    IN6_IS_ADDR_LINKLOCAL(&SA2SIN6ADDR(hint_sa))) {
 				if (SA2SIN6(ifsa)->sin6_scope_id ==
 				    SA2SIN6(caller_sa)->sin6_scope_id) {
-					bestif = ifap;
-					goto found;
+					const int goodness = 3;
+
+					if (bestif_goodness < goodness) {
+						bestif = ifap;
+						bestif_goodness = goodness;
+					}
 				}
-			} else if (!bitmaskcmp(&SA2SIN6ADDR(ifsa),
-			    &SA2SIN6ADDR(hint_sa), &SA2SIN6ADDR(ifmasksa),
-			    sizeof(struct in6_addr))) {
+			}
+		}
+#endif /* INET6 */
+		if (0 == bitmaskcmp(hint_sa, ifsa, ifmasksa)) {
+			const int goodness = 2;
+
+			if (bestif_goodness < goodness) {
 				bestif = ifap;
-				goto found;
+				bestif_goodness = goodness;
 			}
-			break;
-#endif
-		default:
-			continue;
 		}
+		if (!(ifap->ifa_flags & (IFF_LOOPBACK | IFF_POINTOPOINT))) {
+			const int goodness = 1;
 
-		/*
-		 * Remember the first possibly useful interface, preferring
-		 * "normal" to point-to-point and loopback ones.
-		 */
-		if (bestif == NULL ||
-		    (!(ifap->ifa_flags & (IFF_LOOPBACK | IFF_POINTOPOINT)) &&
-		    (bestif->ifa_flags & (IFF_LOOPBACK | IFF_POINTOPOINT))))
+			if (bestif_goodness < goodness) {
+				bestif = ifap;
+				bestif_goodness = goodness;
+			}
+		}
+		if (bestif == NULL)
 			bestif = ifap;
 	}
 	if (bestif == NULL)