[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180713014910.GC1392@brightrain.aerifal.cx>
Date: Thu, 12 Jul 2018 21:49:10 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: getaddrinfo(3) / AI_ADDRCONFIG
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@...c.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
View attachment "ai_addrconfig.diff" of type "text/plain" (1638 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
