getaddrinfo(3) / AI_ADDRCONFIG

Submitted by Rich Felker on July 13, 2018, 1:49 a.m.

Details

Message ID 20180713014910.GC1392@brightrain.aerifal.cx
State New
Series "getaddrinfo(3) / AI_ADDRCONFIG"
Headers show

Commit Message

Rich Felker July 13, 2018, 1:49 a.m.
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

Patch hide | download patch | download mbox

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;
 

Comments

Christopher Friedt July 13, 2018, 2:53 a.m.
On Thu, Jul 12, 2018 at 9:49 PM Rich Felker <dalias@libc.org> wrote:
> 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.

I originally wrote my patch with the intention of being as unobtrusive
as possible but rather than disagree realized it was better to just do
what you wanted me to.

The struct was a better solution for when the addrconfig logic lived
in a separate function. It probably could have even been a separate
static function inside of getaddrinfo.c, but I anticipated that you
would not have liked that.

Definitely correct to disable pthread cancellation.

I used struct sockaddr_storage to avoid declaring more than one
sockaddr because I thought you would have preferred that. Personally,
I would have preferred to use two separate sockaddr too. Solves that
problem.

Originally, I wanted to use a loop over the length of a table, but
figured you would dislike that in favour of readability. Assuming
there will only be ever be AF_INET and AF_INET6 support for
getaddrinfo(3), handling it this or that way is fine.

The patch works for me as is or with the loop.

C