Re: getaddrinfo(3) / AI_ADDRCONFIG

[Rich Felker <dalias@libc.org>]

[-- Attachment #1: Type: text/plain, Size: 5714 bytes --]

On Wed, Jul 11, 2018 at 09:20:20PM -0400, Christopher Friedt wrote:
> On Wed, Jul 11, 2018 at 1:00 PM Rich Felker <dalias@libc.org> wrote:
> > It could probably go inside __lookup_name, but maybe in getaddrinfo is
> > better since that would avoid linking it for gethostbyname, etc.
> > (which don't need it).
> 
> Done
> 
> > > inconsistent with musl (spaces
> > > > after opening and before closing paren, etc.).
> 
> Done
> 
> > I think the one mandated by POSIX is EAI_NONAME ("The name does not
> > resolve for the supplied parameters").
> 
> Done

Something's not going right with our communication about this. I've
attached an untested patch that's closer to what I've been looking
for. It corrects an oversight I'd made, that we need to block
cancellation during the probe, and localizes the change as originally
requested. Please let me know if it works. Arguably it might be nicer
to replace the repeated code with a table and 2-iteration for loop.

Also:

> diff --git a/src/network/getaddrinfo.c b/src/network/getaddrinfo.c
> index b9439f77..72276ddc 100644
> --- a/src/network/getaddrinfo.c
> +++ b/src/network/getaddrinfo.c
> @@ -1,8 +1,10 @@
> +#include <stdbool.h>
>  #include <stdlib.h>
>  #include <sys/socket.h>
>  #include <netinet/in.h>
>  #include <netdb.h>
>  #include <string.h>
> +#include <unistd.h>
>  #include "lookup.h"
>  
>  int getaddrinfo(const char *restrict host, const char *restrict serv, const struct addrinfo *restrict hint, struct addrinfo **restrict res)
> @@ -10,7 +12,7 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
>  	struct service ports[MAXSERVS];
>  	struct address addrs[MAXADDRS];
>  	char canon[256], *outcanon;
> -	int nservs, naddrs, nais, canon_len, i, j, k;
> +	int nservs, naddrs, nais, canon_len, i, j, k, fd, r;
>  	int family = AF_UNSPEC, flags = 0, proto = 0, socktype = 0;
>  	struct aibuf {
>  		struct addrinfo ai;
> @@ -19,6 +21,11 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
>  			struct sockaddr_in6 sin6;
>  		} sa;
>  	} *out;
> +	struct addrconfig {
> +		bool af_inet;
> +		bool af_inet6;
> +	} addrconfig;

This struct, and use of bool, is completely gratuitous.

> +	struct sockaddr_storage sas;

Use of sockaddr_storage is pretty much always a bug.

>  	if (!host && !serv) return EAI_NONAME;
>  
> @@ -33,6 +40,35 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
>  		if ((flags & mask) != flags)
>  			return EAI_BADFLAGS;
>  
> +		if (flags & AI_ADDRCONFIG) {
> +			*((struct sockaddr_in *)(&sas)) = (struct sockaddr_in){
> +				.sin_family = AF_INET,
> +				.sin_port = htons(42),
> +				.sin_addr.s_addr = INADDR_LOOPBACK,
> +			};

And indeed here undefined behavior is invoked by storing with an
lvalue whose type does not match the type of the object.

> +			addrconfig.af_inet = false;
> +			r = socket(AF_INET, SOCK_DGRAM, 0);
> +			if (-1 != r) {
> +				fd = r;
> +				r = connect(fd, (struct sockaddr *) & sas, sizeof( struct sockaddr_in ));
> +				addrconfig.af_inet = 0 == r;
> +				close(fd);
> +			}
> +			*((struct sockaddr_in6 *)(&sas)) = (struct sockaddr_in6){
> +				.sin6_family = AF_INET6,
> +				.sin6_port = htons(42),
> +				.sin6_addr = IN6ADDR_LOOPBACK_INIT,
> +			};
> +			addrconfig.af_inet6 = false;
> +			r = socket(AF_INET6, SOCK_DGRAM, 0);
> +			if (-1 != r) {
> +				fd = r;
> +				r = connect(fd, (struct sockaddr *) & sas, sizeof(struct sockaddr_in6));
> +				addrconfig.af_inet6 = 0 == r;
> +				close(fd);
> +			}
> +		}
> +
>  		switch (family) {
>  		case AF_INET:
>  		case AF_INET6:
> @@ -61,30 +97,43 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
>  		outcanon = 0;
>  	}
>  
> -	for (k=i=0; i<naddrs; i++) for (j=0; j<nservs; j++, k++) {
> -		out[k].ai = (struct addrinfo){
> -			.ai_family = addrs[i].family,
> -			.ai_socktype = ports[j].socktype,
> -			.ai_protocol = ports[j].proto,
> -			.ai_addrlen = addrs[i].family == AF_INET
> -				? sizeof(struct sockaddr_in)
> -				: sizeof(struct sockaddr_in6),
> -			.ai_addr = (void *)&out[k].sa,
> -			.ai_canonname = outcanon,
> -			.ai_next = &out[k+1].ai };
> +	for (k=i=0; i<naddrs; i++) for (j=0; j<nservs; j++) {
>  		switch (addrs[i].family) {
>  		case AF_INET:
> +			if ((flags & AI_ADDRCONFIG) && !addrconfig.af_inet) {
> +				nais--;
> +				continue;
> +			}
>  			out[k].sa.sin.sin_family = AF_INET;
>  			out[k].sa.sin.sin_port = htons(ports[j].port);
>  			memcpy(&out[k].sa.sin.sin_addr, &addrs[i].addr, 4);
>  			break;
>  		case AF_INET6:
> +			if ((flags & AI_ADDRCONFIG ) && !addrconfig.af_inet6) {
> +				nais--;
> +				continue;
> +			}
>  			out[k].sa.sin6.sin6_family = AF_INET6;
>  			out[k].sa.sin6.sin6_port = htons(ports[j].port);
>  			out[k].sa.sin6.sin6_scope_id = addrs[i].scopeid;
>  			memcpy(&out[k].sa.sin6.sin6_addr, &addrs[i].addr, 16);
> -			break;			
> +			break;
>  		}
> +		out[k].ai = (struct addrinfo){
> +			.ai_family = addrs[i].family,
> +			.ai_socktype = ports[j].socktype,
> +			.ai_protocol = ports[j].proto,
> +			.ai_addrlen = addrs[i].family == AF_INET
> +				? sizeof(struct sockaddr_in)
> +				: sizeof(struct sockaddr_in6),
> +			.ai_addr = (void *)&out[k].sa,
> +			.ai_canonname = outcanon,
> +			.ai_next = &out[k+1].ai };
> +		k++;
> +	}
> +	if ( nais < 1 ) {
> +		free( out );
> +		return EAI_NONAME;
>  	}
>  	out[nais-1].ai.ai_next = 0;
>  	*res = &out->ai;

All of this is unnecessary, and fails the main legitimate purpose of
AI_ADDRCONFIG, which is suppressing DNS queries that are known not to
be needed. Instead family should simply be remapped before calling
__lookup_name, as I suggested a few emails back in this thread.

Rich

[-- Attachment #2: ai_addrconfig.diff --]
[-- Type: text/plain, Size: 1637 bytes --]

diff --git a/src/network/getaddrinfo.c b/src/network/getaddrinfo.c
index b9439f7..664a72a 100644
--- a/src/network/getaddrinfo.c
+++ b/src/network/getaddrinfo.c
@@ -3,6 +3,8 @@
 #include <netinet/in.h>
 #include <netdb.h>
 #include <string.h>
+#include <pthread.h>
+#include <unistd.h>
 #include "lookup.h"
 
 int getaddrinfo(const char *restrict host, const char *restrict serv, const struct addrinfo *restrict hint, struct addrinfo **restrict res)
@@ -43,6 +45,46 @@ int getaddrinfo(const char *restrict host, const char *restrict serv, const stru
 		}
 	}
 
+	if (flags & AI_ADDRCONFIG) {
+		int cs;
+		pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
+		if (family != AF_INET6) {
+			struct sockaddr_in sa4 = {
+				.sin_family = AF_INET,
+				.sin_port = 65535,
+				.sin_addr.s_addr = INADDR_LOOPBACK
+			};
+			int s = socket(AF_INET, SOCK_CLOEXEC|SOCK_DGRAM,
+				IPPROTO_UDP);
+			if (s>=0) {
+				int r = connect(s, (void *)&sa4, sizeof sa4);
+				close(s);
+				if (r) {
+					if (family == AF_INET) return EAI_NONAME;
+					family = AF_INET6;
+				}
+			}
+		}
+		if (family != AF_INET) {
+			struct sockaddr_in6 sa6 = {
+				.sin6_family = AF_INET6,
+				.sin6_port = 65535,
+				.sin6_addr = IN6ADDR_LOOPBACK_INIT
+			};
+			int s = socket(AF_INET6, SOCK_CLOEXEC|SOCK_DGRAM,
+				IPPROTO_UDP);
+			if (s>=0) {
+				int r = connect(s, (void *)&sa6, sizeof sa6);
+				close(s);
+				if (r) {
+					if (family == AF_INET6) return EAI_NONAME;
+					family = AF_INET;
+				}
+			}
+		}
+		pthread_setcancelstate(cs, 0);
+	}
+
 	nservs = __lookup_serv(ports, serv, proto, socktype, flags);
 	if (nservs < 0) return nservs;