Message ID | 20140415162902.GA8469@spoyarek.pnq.redhat.com |
---|---|
State | New |
Headers | show |
Forgot one detail: 2014-04-16 Siddhesh Poyarekar <siddhesh@redhat.com> Atsushi Onoe <atsushi@onoe.org> > > [BZ #14308] > * resolv/res_query.c (__libc_res_nsearch): Return if at least > one response is valid. > * resolv/res_send.c (send_dg): Check for validity of other > response if the current response is a referral. > > --- > resolv/res_query.c | 7 +++++-- > resolv/res_send.c | 2 +- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/resolv/res_query.c b/resolv/res_query.c > index a9db837..4e6612c 100644 > --- a/resolv/res_query.c > +++ b/resolv/res_query.c > @@ -382,7 +382,9 @@ __libc_res_nsearch(res_state statp, > answer, anslen, answerp, > answerp2, nanswerp2, resplen2, > answerp2_malloced); > - if (ret > 0 || trailing_dot) > + if (ret > 0 || trailing_dot > + /* If the second response is valid then we use that. */ > + || (ret == 0 && answerp2 != NULL && resplen2 > 0)) > return (ret); > saved_herrno = h_errno; > tried_as_is++; > @@ -422,7 +424,8 @@ __libc_res_nsearch(res_state statp, > answer, anslen, answerp, > answerp2, nanswerp2, > resplen2, answerp2_malloced); > - if (ret > 0) > + if (ret > 0 || (ret == 0 && answerp2 != NULL > + && resplen2 > 0)) > return (ret); > > if (answerp && *answerp != answer) { > diff --git a/resolv/res_send.c b/resolv/res_send.c > index 60743df..3273d55 100644 > --- a/resolv/res_send.c > +++ b/resolv/res_send.c > @@ -1351,6 +1351,7 @@ send_dg(res_state statp, > (*thisresplenp > *thisanssizp) > ? *thisanssizp : *thisresplenp); > > + next_ns: > if (recvresp1 || (buf2 != NULL && recvresp2)) { > *resplen2 = 0; > return resplen; > @@ -1368,7 +1369,6 @@ send_dg(res_state statp, > goto wait; > } > > - next_ns: > __res_iclose(statp, false); > /* don't retry if called from dig */ > if (!statp->pfcode) > -- > 1.8.3.1 >
Incidentally, this also fixes bz #12994. Siddhesh On Tue, Apr 15, 2014 at 09:59:02PM +0530, Siddhesh Poyarekar wrote: > AF_UNSPEC results in sending two queries in parallel, one for the A > record and the other for the AAAA record. If one of these is a > referral, then the query fails, which is wrong. It should return at > least the one successful response. > > The fix has two parts. The first part makes the referral fall back to > the SERVFAIL path, which results in using the successful response. > There is a bug in that path however, due to which the second part is > necessary. The bug here is that if the first response is a failure > and the second succeeds, __libc_res_nsearch does not detect that and > assumes a failure. The case where the first response is a success and > the second fails, works correctly. > > This condition is produced by buggy routers, so here's a crude > interposable library that can simulate such a condition. The library > overrides the recvfrom syscall and modifies the header of the packet > received to reproduce this scenario. It has two key variables: > mod_packet and first_error. > > The mod_packet variable when set to 0, results in odd packets being > modified to be a referral. When set to 1, even packets are modified > to be a referral. > > The first_error causes the first response to be a failure so that a > domain-appended search is performed to test the second part of the > __libc_nsearch fix. > > The driver for this fix is a simple getaddrinfo program that does an > AF_UNSPEC query. I have omitted this since it should be easy to > implement. > > I have tested this on x86_64. > > The interceptor library source: > > /* Override recvfrom and modify the header of the first DNS response to make it > a referral and reproduce bz #845218. We have to resort to this ugly hack > because we cannot make bind return the buggy response of a referral for the > AAAA record and an authoritative response for the A record. */ > #define _GNU_SOURCE > #include <sys/types.h> > #include <sys/socket.h> > #include <netinet/in.h> > #include <arpa/inet.h> > #include <stdio.h> > #include <stdbool.h> > #include <endian.h> > #include <dlfcn.h> > #include <stdlib.h> > > /* Lifted from resolv/arpa/nameser_compat.h. */ > typedef struct { > unsigned id :16; /*%< query identification number */ > #if BYTE_ORDER == BIG_ENDIAN > /* fields in third byte */ > unsigned qr: 1; /*%< response flag */ > unsigned opcode: 4; /*%< purpose of message */ > unsigned aa: 1; /*%< authoritive answer */ > unsigned tc: 1; /*%< truncated message */ > unsigned rd: 1; /*%< recursion desired */ > /* fields > * in > * fourth > * byte > * */ > unsigned ra: 1; /*%< recursion available */ > unsigned unused :1; /*%< unused bits (MBZ as of 4.9.3a3) */ > unsigned ad: 1; /*%< authentic data from named */ > unsigned cd: 1; /*%< checking disabled by resolver */ > unsigned rcode :4; /*%< response code */ > #endif > #if BYTE_ORDER == LITTLE_ENDIAN || BYTE_ORDER == PDP_ENDIAN > /* fields > * in > * third > * byte > * */ > unsigned rd :1; /*%< recursion desired */ > unsigned tc :1; /*%< truncated message */ > unsigned aa :1; /*%< authoritive answer */ > unsigned opcode :4; /*%< purpose of message */ > unsigned qr :1; /*%< response flag */ > /* fields > * in > * fourth > * byte > * */ > unsigned rcode :4; /*%< response code */ > unsigned cd: 1; /*%< checking disabled by resolver */ > unsigned ad: 1; /*%< authentic data from named */ > unsigned unused :1; /*%< unused bits (MBZ as of 4.9.3a3) */ > unsigned ra :1; /*%< recursion available */ > #endif > /* remaining > * bytes > * */ > unsigned qdcount :16; /*%< number of question entries */ > unsigned ancount :16; /*%< number of answer entries */ > unsigned nscount :16; /*%< number of authority entries */ > unsigned arcount :16; /*%< number of resource entries */ > } HEADER; > > static int done = 0; > > /* Packets to modify. 0 for the odd packets and 1 for even packets. */ > static const int mod_packet = 0; > > /* Set to true if the first request should result in an error, resulting in a > search query. */ > static bool first_error = true; > > static ssize_t (*real_recvfrom) (int sockfd, void *buf, size_t len, int flags, > struct sockaddr *src_addr, socklen_t *addrlen); > > void > __attribute__ ((constructor)) > init (void) > { > real_recvfrom = dlsym (RTLD_NEXT, "recvfrom"); > > if (real_recvfrom == NULL) > { > printf ("Failed to get reference to recvfrom: %s\n", dlerror ()); > printf ("Cannot simulate test\n"); > abort (); > } > } > > /* Modify the second packet that we receive to set the header in a manner as to > reproduce BZ #845218. */ > static void > mod_buf (HEADER *h, int port) > { > if (done % 2 == mod_packet || (first_error && done == 1)) > { > printf ("(Modifying header)"); > > if (first_error && done == 1) > h->rcode = 3; > else > h->rcode = 0; /* NOERROR == 0. */ > h->ancount = 0; > h->aa = 0; > h->ra = 0; > h->arcount = 0; > } > done++; > } > > ssize_t > recvfrom (int sockfd, void *buf, size_t len, int flags, > struct sockaddr *src_addr, socklen_t *addrlen) > { > ssize_t ret = real_recvfrom (sockfd, buf, len, flags, src_addr, addrlen); > int port = htons (((struct sockaddr_in *) src_addr)->sin_port); > struct in_addr addr = ((struct sockaddr_in *) src_addr)->sin_addr; > const char *host = inet_ntoa (addr); > printf ("\n*** From %s:%d: ", host, port); > > mod_buf (buf, port); > > printf ("returned %zd\n", ret); > return ret; > } > > > [BZ #14308] > * resolv/res_query.c (__libc_res_nsearch): Return if at least > one response is valid. > * resolv/res_send.c (send_dg): Check for validity of other > response if the current response is a referral. > > --- > resolv/res_query.c | 7 +++++-- > resolv/res_send.c | 2 +- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/resolv/res_query.c b/resolv/res_query.c > index a9db837..4e6612c 100644 > --- a/resolv/res_query.c > +++ b/resolv/res_query.c > @@ -382,7 +382,9 @@ __libc_res_nsearch(res_state statp, > answer, anslen, answerp, > answerp2, nanswerp2, resplen2, > answerp2_malloced); > - if (ret > 0 || trailing_dot) > + if (ret > 0 || trailing_dot > + /* If the second response is valid then we use that. */ > + || (ret == 0 && answerp2 != NULL && resplen2 > 0)) > return (ret); > saved_herrno = h_errno; > tried_as_is++; > @@ -422,7 +424,8 @@ __libc_res_nsearch(res_state statp, > answer, anslen, answerp, > answerp2, nanswerp2, > resplen2, answerp2_malloced); > - if (ret > 0) > + if (ret > 0 || (ret == 0 && answerp2 != NULL > + && resplen2 > 0)) > return (ret); > > if (answerp && *answerp != answer) { > diff --git a/resolv/res_send.c b/resolv/res_send.c > index 60743df..3273d55 100644 > --- a/resolv/res_send.c > +++ b/resolv/res_send.c > @@ -1351,6 +1351,7 @@ send_dg(res_state statp, > (*thisresplenp > *thisanssizp) > ? *thisanssizp : *thisresplenp); > > + next_ns: > if (recvresp1 || (buf2 != NULL && recvresp2)) { > *resplen2 = 0; > return resplen; > @@ -1368,7 +1369,6 @@ send_dg(res_state statp, > goto wait; > } > > - next_ns: > __res_iclose(statp, false); > /* don't retry if called from dig */ > if (!statp->pfcode) > -- > 1.8.3.1 >
Siddhesh Poyarekar <siddhesh@redhat.com> writes:
> Incidentally, this also fixes bz #12994.
And 13651?
Andreas.
On Wed, Apr 16, 2014 at 10:57:31AM +0200, Andreas Schwab wrote: > Siddhesh Poyarekar <siddhesh@redhat.com> writes: > > > Incidentally, this also fixes bz #12994. > > And 13651? > Oh yeah, in fact I think it's a duplicate. Siddhesh
Ping! On Tue, Apr 15, 2014 at 09:59:02PM +0530, Siddhesh Poyarekar wrote: > AF_UNSPEC results in sending two queries in parallel, one for the A > record and the other for the AAAA record. If one of these is a > referral, then the query fails, which is wrong. It should return at > least the one successful response. > > The fix has two parts. The first part makes the referral fall back to > the SERVFAIL path, which results in using the successful response. > There is a bug in that path however, due to which the second part is > necessary. The bug here is that if the first response is a failure > and the second succeeds, __libc_res_nsearch does not detect that and > assumes a failure. The case where the first response is a success and > the second fails, works correctly. > > This condition is produced by buggy routers, so here's a crude > interposable library that can simulate such a condition. The library > overrides the recvfrom syscall and modifies the header of the packet > received to reproduce this scenario. It has two key variables: > mod_packet and first_error. > > The mod_packet variable when set to 0, results in odd packets being > modified to be a referral. When set to 1, even packets are modified > to be a referral. > > The first_error causes the first response to be a failure so that a > domain-appended search is performed to test the second part of the > __libc_nsearch fix. > > The driver for this fix is a simple getaddrinfo program that does an > AF_UNSPEC query. I have omitted this since it should be easy to > implement. > > I have tested this on x86_64. > > The interceptor library source: > > /* Override recvfrom and modify the header of the first DNS response to make it > a referral and reproduce bz #845218. We have to resort to this ugly hack > because we cannot make bind return the buggy response of a referral for the > AAAA record and an authoritative response for the A record. */ > #define _GNU_SOURCE > #include <sys/types.h> > #include <sys/socket.h> > #include <netinet/in.h> > #include <arpa/inet.h> > #include <stdio.h> > #include <stdbool.h> > #include <endian.h> > #include <dlfcn.h> > #include <stdlib.h> > > /* Lifted from resolv/arpa/nameser_compat.h. */ > typedef struct { > unsigned id :16; /*%< query identification number */ > #if BYTE_ORDER == BIG_ENDIAN > /* fields in third byte */ > unsigned qr: 1; /*%< response flag */ > unsigned opcode: 4; /*%< purpose of message */ > unsigned aa: 1; /*%< authoritive answer */ > unsigned tc: 1; /*%< truncated message */ > unsigned rd: 1; /*%< recursion desired */ > /* fields > * in > * fourth > * byte > * */ > unsigned ra: 1; /*%< recursion available */ > unsigned unused :1; /*%< unused bits (MBZ as of 4.9.3a3) */ > unsigned ad: 1; /*%< authentic data from named */ > unsigned cd: 1; /*%< checking disabled by resolver */ > unsigned rcode :4; /*%< response code */ > #endif > #if BYTE_ORDER == LITTLE_ENDIAN || BYTE_ORDER == PDP_ENDIAN > /* fields > * in > * third > * byte > * */ > unsigned rd :1; /*%< recursion desired */ > unsigned tc :1; /*%< truncated message */ > unsigned aa :1; /*%< authoritive answer */ > unsigned opcode :4; /*%< purpose of message */ > unsigned qr :1; /*%< response flag */ > /* fields > * in > * fourth > * byte > * */ > unsigned rcode :4; /*%< response code */ > unsigned cd: 1; /*%< checking disabled by resolver */ > unsigned ad: 1; /*%< authentic data from named */ > unsigned unused :1; /*%< unused bits (MBZ as of 4.9.3a3) */ > unsigned ra :1; /*%< recursion available */ > #endif > /* remaining > * bytes > * */ > unsigned qdcount :16; /*%< number of question entries */ > unsigned ancount :16; /*%< number of answer entries */ > unsigned nscount :16; /*%< number of authority entries */ > unsigned arcount :16; /*%< number of resource entries */ > } HEADER; > > static int done = 0; > > /* Packets to modify. 0 for the odd packets and 1 for even packets. */ > static const int mod_packet = 0; > > /* Set to true if the first request should result in an error, resulting in a > search query. */ > static bool first_error = true; > > static ssize_t (*real_recvfrom) (int sockfd, void *buf, size_t len, int flags, > struct sockaddr *src_addr, socklen_t *addrlen); > > void > __attribute__ ((constructor)) > init (void) > { > real_recvfrom = dlsym (RTLD_NEXT, "recvfrom"); > > if (real_recvfrom == NULL) > { > printf ("Failed to get reference to recvfrom: %s\n", dlerror ()); > printf ("Cannot simulate test\n"); > abort (); > } > } > > /* Modify the second packet that we receive to set the header in a manner as to > reproduce BZ #845218. */ > static void > mod_buf (HEADER *h, int port) > { > if (done % 2 == mod_packet || (first_error && done == 1)) > { > printf ("(Modifying header)"); > > if (first_error && done == 1) > h->rcode = 3; > else > h->rcode = 0; /* NOERROR == 0. */ > h->ancount = 0; > h->aa = 0; > h->ra = 0; > h->arcount = 0; > } > done++; > } > > ssize_t > recvfrom (int sockfd, void *buf, size_t len, int flags, > struct sockaddr *src_addr, socklen_t *addrlen) > { > ssize_t ret = real_recvfrom (sockfd, buf, len, flags, src_addr, addrlen); > int port = htons (((struct sockaddr_in *) src_addr)->sin_port); > struct in_addr addr = ((struct sockaddr_in *) src_addr)->sin_addr; > const char *host = inet_ntoa (addr); > printf ("\n*** From %s:%d: ", host, port); > > mod_buf (buf, port); > > printf ("returned %zd\n", ret); > return ret; > } > > > [BZ #14308] > * resolv/res_query.c (__libc_res_nsearch): Return if at least > one response is valid. > * resolv/res_send.c (send_dg): Check for validity of other > response if the current response is a referral. > > --- > resolv/res_query.c | 7 +++++-- > resolv/res_send.c | 2 +- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/resolv/res_query.c b/resolv/res_query.c > index a9db837..4e6612c 100644 > --- a/resolv/res_query.c > +++ b/resolv/res_query.c > @@ -382,7 +382,9 @@ __libc_res_nsearch(res_state statp, > answer, anslen, answerp, > answerp2, nanswerp2, resplen2, > answerp2_malloced); > - if (ret > 0 || trailing_dot) > + if (ret > 0 || trailing_dot > + /* If the second response is valid then we use that. */ > + || (ret == 0 && answerp2 != NULL && resplen2 > 0)) > return (ret); > saved_herrno = h_errno; > tried_as_is++; > @@ -422,7 +424,8 @@ __libc_res_nsearch(res_state statp, > answer, anslen, answerp, > answerp2, nanswerp2, > resplen2, answerp2_malloced); > - if (ret > 0) > + if (ret > 0 || (ret == 0 && answerp2 != NULL > + && resplen2 > 0)) > return (ret); > > if (answerp && *answerp != answer) { > diff --git a/resolv/res_send.c b/resolv/res_send.c > index 60743df..3273d55 100644 > --- a/resolv/res_send.c > +++ b/resolv/res_send.c > @@ -1351,6 +1351,7 @@ send_dg(res_state statp, > (*thisresplenp > *thisanssizp) > ? *thisanssizp : *thisresplenp); > > + next_ns: > if (recvresp1 || (buf2 != NULL && recvresp2)) { > *resplen2 = 0; > return resplen; > @@ -1368,7 +1369,6 @@ send_dg(res_state statp, > goto wait; > } > > - next_ns: > __res_iclose(statp, false); > /* don't retry if called from dig */ > if (!statp->pfcode) > -- > 1.8.3.1 >
Ping! On Tue, Apr 22, 2014 at 03:16:00PM +0530, Siddhesh Poyarekar wrote: > Ping! > > On Tue, Apr 15, 2014 at 09:59:02PM +0530, Siddhesh Poyarekar wrote: > > AF_UNSPEC results in sending two queries in parallel, one for the A > > record and the other for the AAAA record. If one of these is a > > referral, then the query fails, which is wrong. It should return at > > least the one successful response. > > > > The fix has two parts. The first part makes the referral fall back to > > the SERVFAIL path, which results in using the successful response. > > There is a bug in that path however, due to which the second part is > > necessary. The bug here is that if the first response is a failure > > and the second succeeds, __libc_res_nsearch does not detect that and > > assumes a failure. The case where the first response is a success and > > the second fails, works correctly. > > > > This condition is produced by buggy routers, so here's a crude > > interposable library that can simulate such a condition. The library > > overrides the recvfrom syscall and modifies the header of the packet > > received to reproduce this scenario. It has two key variables: > > mod_packet and first_error. > > > > The mod_packet variable when set to 0, results in odd packets being > > modified to be a referral. When set to 1, even packets are modified > > to be a referral. > > > > The first_error causes the first response to be a failure so that a > > domain-appended search is performed to test the second part of the > > __libc_nsearch fix. > > > > The driver for this fix is a simple getaddrinfo program that does an > > AF_UNSPEC query. I have omitted this since it should be easy to > > implement. > > > > I have tested this on x86_64. > > > > The interceptor library source: > > > > /* Override recvfrom and modify the header of the first DNS response to make it > > a referral and reproduce bz #845218. We have to resort to this ugly hack > > because we cannot make bind return the buggy response of a referral for the > > AAAA record and an authoritative response for the A record. */ > > #define _GNU_SOURCE > > #include <sys/types.h> > > #include <sys/socket.h> > > #include <netinet/in.h> > > #include <arpa/inet.h> > > #include <stdio.h> > > #include <stdbool.h> > > #include <endian.h> > > #include <dlfcn.h> > > #include <stdlib.h> > > > > /* Lifted from resolv/arpa/nameser_compat.h. */ > > typedef struct { > > unsigned id :16; /*%< query identification number */ > > #if BYTE_ORDER == BIG_ENDIAN > > /* fields in third byte */ > > unsigned qr: 1; /*%< response flag */ > > unsigned opcode: 4; /*%< purpose of message */ > > unsigned aa: 1; /*%< authoritive answer */ > > unsigned tc: 1; /*%< truncated message */ > > unsigned rd: 1; /*%< recursion desired */ > > /* fields > > * in > > * fourth > > * byte > > * */ > > unsigned ra: 1; /*%< recursion available */ > > unsigned unused :1; /*%< unused bits (MBZ as of 4.9.3a3) */ > > unsigned ad: 1; /*%< authentic data from named */ > > unsigned cd: 1; /*%< checking disabled by resolver */ > > unsigned rcode :4; /*%< response code */ > > #endif > > #if BYTE_ORDER == LITTLE_ENDIAN || BYTE_ORDER == PDP_ENDIAN > > /* fields > > * in > > * third > > * byte > > * */ > > unsigned rd :1; /*%< recursion desired */ > > unsigned tc :1; /*%< truncated message */ > > unsigned aa :1; /*%< authoritive answer */ > > unsigned opcode :4; /*%< purpose of message */ > > unsigned qr :1; /*%< response flag */ > > /* fields > > * in > > * fourth > > * byte > > * */ > > unsigned rcode :4; /*%< response code */ > > unsigned cd: 1; /*%< checking disabled by resolver */ > > unsigned ad: 1; /*%< authentic data from named */ > > unsigned unused :1; /*%< unused bits (MBZ as of 4.9.3a3) */ > > unsigned ra :1; /*%< recursion available */ > > #endif > > /* remaining > > * bytes > > * */ > > unsigned qdcount :16; /*%< number of question entries */ > > unsigned ancount :16; /*%< number of answer entries */ > > unsigned nscount :16; /*%< number of authority entries */ > > unsigned arcount :16; /*%< number of resource entries */ > > } HEADER; > > > > static int done = 0; > > > > /* Packets to modify. 0 for the odd packets and 1 for even packets. */ > > static const int mod_packet = 0; > > > > /* Set to true if the first request should result in an error, resulting in a > > search query. */ > > static bool first_error = true; > > > > static ssize_t (*real_recvfrom) (int sockfd, void *buf, size_t len, int flags, > > struct sockaddr *src_addr, socklen_t *addrlen); > > > > void > > __attribute__ ((constructor)) > > init (void) > > { > > real_recvfrom = dlsym (RTLD_NEXT, "recvfrom"); > > > > if (real_recvfrom == NULL) > > { > > printf ("Failed to get reference to recvfrom: %s\n", dlerror ()); > > printf ("Cannot simulate test\n"); > > abort (); > > } > > } > > > > /* Modify the second packet that we receive to set the header in a manner as to > > reproduce BZ #845218. */ > > static void > > mod_buf (HEADER *h, int port) > > { > > if (done % 2 == mod_packet || (first_error && done == 1)) > > { > > printf ("(Modifying header)"); > > > > if (first_error && done == 1) > > h->rcode = 3; > > else > > h->rcode = 0; /* NOERROR == 0. */ > > h->ancount = 0; > > h->aa = 0; > > h->ra = 0; > > h->arcount = 0; > > } > > done++; > > } > > > > ssize_t > > recvfrom (int sockfd, void *buf, size_t len, int flags, > > struct sockaddr *src_addr, socklen_t *addrlen) > > { > > ssize_t ret = real_recvfrom (sockfd, buf, len, flags, src_addr, addrlen); > > int port = htons (((struct sockaddr_in *) src_addr)->sin_port); > > struct in_addr addr = ((struct sockaddr_in *) src_addr)->sin_addr; > > const char *host = inet_ntoa (addr); > > printf ("\n*** From %s:%d: ", host, port); > > > > mod_buf (buf, port); > > > > printf ("returned %zd\n", ret); > > return ret; > > } > > > > > > [BZ #14308] > > * resolv/res_query.c (__libc_res_nsearch): Return if at least > > one response is valid. > > * resolv/res_send.c (send_dg): Check for validity of other > > response if the current response is a referral. > > > > --- > > resolv/res_query.c | 7 +++++-- > > resolv/res_send.c | 2 +- > > 2 files changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/resolv/res_query.c b/resolv/res_query.c > > index a9db837..4e6612c 100644 > > --- a/resolv/res_query.c > > +++ b/resolv/res_query.c > > @@ -382,7 +382,9 @@ __libc_res_nsearch(res_state statp, > > answer, anslen, answerp, > > answerp2, nanswerp2, resplen2, > > answerp2_malloced); > > - if (ret > 0 || trailing_dot) > > + if (ret > 0 || trailing_dot > > + /* If the second response is valid then we use that. */ > > + || (ret == 0 && answerp2 != NULL && resplen2 > 0)) > > return (ret); > > saved_herrno = h_errno; > > tried_as_is++; > > @@ -422,7 +424,8 @@ __libc_res_nsearch(res_state statp, > > answer, anslen, answerp, > > answerp2, nanswerp2, > > resplen2, answerp2_malloced); > > - if (ret > 0) > > + if (ret > 0 || (ret == 0 && answerp2 != NULL > > + && resplen2 > 0)) > > return (ret); > > > > if (answerp && *answerp != answer) { > > diff --git a/resolv/res_send.c b/resolv/res_send.c > > index 60743df..3273d55 100644 > > --- a/resolv/res_send.c > > +++ b/resolv/res_send.c > > @@ -1351,6 +1351,7 @@ send_dg(res_state statp, > > (*thisresplenp > *thisanssizp) > > ? *thisanssizp : *thisresplenp); > > > > + next_ns: > > if (recvresp1 || (buf2 != NULL && recvresp2)) { > > *resplen2 = 0; > > return resplen; > > @@ -1368,7 +1369,6 @@ send_dg(res_state statp, > > goto wait; > > } > > > > - next_ns: > > __res_iclose(statp, false); > > /* don't retry if called from dig */ > > if (!statp->pfcode) > > -- > > 1.8.3.1 > > > >
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Siddhesh, Review below. Summary: Looks good to me and makes sense. I like the clever trick of moving "next_ns" up to check the second response :-) On 04/15/2014 12:29 PM, Siddhesh Poyarekar wrote: > AF_UNSPEC results in sending two queries in parallel, one for the A > record and the other for the AAAA record. If one of these is a > referral, then the query fails, which is wrong. It should return at > least the one successful response. That's correct. From first principles a stub resolver can't handle the referral response as it's not a recursive resolver. > The fix has two parts. The first part makes the referral fall back to > the SERVFAIL path, which results in using the successful response. > There is a bug in that path however, due to which the second part is > necessary. The bug here is that if the first response is a failure > and the second succeeds, __libc_res_nsearch does not detect that and > assumes a failure. The case where the first response is a success and > the second fails, works correctly. OK. > This condition is produced by buggy routers, so here's a crude > interposable library that can simulate such a condition. The library > overrides the recvfrom syscall and modifies the header of the packet > received to reproduce this scenario. It has two key variables: > mod_packet and first_error. Thanks, that's very useful. > The mod_packet variable when set to 0, results in odd packets being > modified to be a referral. When set to 1, even packets are modified > to be a referral. > > The first_error causes the first response to be a failure so that a > domain-appended search is performed to test the second part of the > __libc_nsearch fix. OK. > The driver for this fix is a simple getaddrinfo program that does an > AF_UNSPEC query. I have omitted this since it should be easy to > implement. > > I have tested this on x86_64. > > The interceptor library source: > > /* Override recvfrom and modify the header of the first DNS response to make it > a referral and reproduce bz #845218. We have to resort to this ugly hack > because we cannot make bind return the buggy response of a referral for the > AAAA record and an authoritative response for the A record. */ > #define _GNU_SOURCE > #include <sys/types.h> > #include <sys/socket.h> > #include <netinet/in.h> > #include <arpa/inet.h> > #include <stdio.h> > #include <stdbool.h> > #include <endian.h> > #include <dlfcn.h> > #include <stdlib.h> > > /* Lifted from resolv/arpa/nameser_compat.h. */ > typedef struct { > unsigned id :16; /*%< query identification number */ > #if BYTE_ORDER == BIG_ENDIAN > /* fields in third byte */ > unsigned qr: 1; /*%< response flag */ > unsigned opcode: 4; /*%< purpose of message */ > unsigned aa: 1; /*%< authoritive answer */ > unsigned tc: 1; /*%< truncated message */ > unsigned rd: 1; /*%< recursion desired */ > /* fields > * in > * fourth > * byte > * */ > unsigned ra: 1; /*%< recursion available */ > unsigned unused :1; /*%< unused bits (MBZ as of 4.9.3a3) */ > unsigned ad: 1; /*%< authentic data from named */ > unsigned cd: 1; /*%< checking disabled by resolver */ > unsigned rcode :4; /*%< response code */ > #endif > #if BYTE_ORDER == LITTLE_ENDIAN || BYTE_ORDER == PDP_ENDIAN > /* fields > * in > * third > * byte > * */ > unsigned rd :1; /*%< recursion desired */ > unsigned tc :1; /*%< truncated message */ > unsigned aa :1; /*%< authoritive answer */ > unsigned opcode :4; /*%< purpose of message */ > unsigned qr :1; /*%< response flag */ > /* fields > * in > * fourth > * byte > * */ > unsigned rcode :4; /*%< response code */ > unsigned cd: 1; /*%< checking disabled by resolver */ > unsigned ad: 1; /*%< authentic data from named */ > unsigned unused :1; /*%< unused bits (MBZ as of 4.9.3a3) */ > unsigned ra :1; /*%< recursion available */ > #endif > /* remaining > * bytes > * */ > unsigned qdcount :16; /*%< number of question entries */ > unsigned ancount :16; /*%< number of answer entries */ > unsigned nscount :16; /*%< number of authority entries */ > unsigned arcount :16; /*%< number of resource entries */ > } HEADER; > > static int done = 0; > > /* Packets to modify. 0 for the odd packets and 1 for even packets. */ > static const int mod_packet = 0; > > /* Set to true if the first request should result in an error, resulting in a > search query. */ > static bool first_error = true; > > static ssize_t (*real_recvfrom) (int sockfd, void *buf, size_t len, int flags, > struct sockaddr *src_addr, socklen_t *addrlen); > > void > __attribute__ ((constructor)) > init (void) > { > real_recvfrom = dlsym (RTLD_NEXT, "recvfrom"); > > if (real_recvfrom == NULL) > { > printf ("Failed to get reference to recvfrom: %s\n", dlerror ()); > printf ("Cannot simulate test\n"); > abort (); > } > } > > /* Modify the second packet that we receive to set the header in a manner as to > reproduce BZ #845218. */ > static void > mod_buf (HEADER *h, int port) > { > if (done % 2 == mod_packet || (first_error && done == 1)) > { > printf ("(Modifying header)"); > > if (first_error && done == 1) > h->rcode = 3; > else > h->rcode = 0; /* NOERROR == 0. */ > h->ancount = 0; > h->aa = 0; > h->ra = 0; > h->arcount = 0; > } > done++; > } > > ssize_t > recvfrom (int sockfd, void *buf, size_t len, int flags, > struct sockaddr *src_addr, socklen_t *addrlen) > { > ssize_t ret = real_recvfrom (sockfd, buf, len, flags, src_addr, addrlen); > int port = htons (((struct sockaddr_in *) src_addr)->sin_port); > struct in_addr addr = ((struct sockaddr_in *) src_addr)->sin_addr; > const char *host = inet_ntoa (addr); > printf ("\n*** From %s:%d: ", host, port); > > mod_buf (buf, port); > > printf ("returned %zd\n", ret); > return ret; > } > > > [BZ #14308] > * resolv/res_query.c (__libc_res_nsearch): Return if at least > one response is valid. > * resolv/res_send.c (send_dg): Check for validity of other > response if the current response is a referral. > > --- > resolv/res_query.c | 7 +++++-- > resolv/res_send.c | 2 +- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/resolv/res_query.c b/resolv/res_query.c > index a9db837..4e6612c 100644 > --- a/resolv/res_query.c > +++ b/resolv/res_query.c > @@ -382,7 +382,9 @@ __libc_res_nsearch(res_state statp, > answer, anslen, answerp, > answerp2, nanswerp2, resplen2, > answerp2_malloced); > - if (ret > 0 || trailing_dot) > + if (ret > 0 || trailing_dot > + /* If the second response is valid then we use that. */ > + || (ret == 0 && answerp2 != NULL && resplen2 > 0)) OK. Verified __libc_res_nquerydomain results in a send_dg or equivalent such that we know `answerp2 != NULL && resplen2 > 0` is going to indicate that we have a second T_AAAA type response back and should check it also. > return (ret); > saved_herrno = h_errno; > tried_as_is++; > @@ -422,7 +424,8 @@ __libc_res_nsearch(res_state statp, > answer, anslen, answerp, > answerp2, nanswerp2, > resplen2, answerp2_malloced); > - if (ret > 0) > + if (ret > 0 || (ret == 0 && answerp2 != NULL > + && resplen2 > 0)) OK. > return (ret); > > if (answerp && *answerp != answer) { > diff --git a/resolv/res_send.c b/resolv/res_send.c > index 60743df..3273d55 100644 > --- a/resolv/res_send.c > +++ b/resolv/res_send.c > @@ -1351,6 +1351,7 @@ send_dg(res_state statp, > (*thisresplenp > *thisanssizp) > ? *thisanssizp : *thisresplenp); > > + next_ns: OK. Verified other paths should be OK with next_ns testing for the second response on SERVFAIL, it increases the latency a little but this is a buggy server and we're trying to salvage the response. > if (recvresp1 || (buf2 != NULL && recvresp2)) { > *resplen2 = 0; > return resplen; > @@ -1368,7 +1369,6 @@ send_dg(res_state statp, > goto wait; > } > > - next_ns: > __res_iclose(statp, false); > /* don't retry if called from dig */ > if (!statp->pfcode) > Looks good to me. Cheers, Carlos. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBAgAGBQJTX3JGAAoJECXvCkNsKkr/xIYH/0XcR+TH4nT2eTiJCn4TXm0K i75dnby1WIEPV6QlR8mmOv+LqZY50mPG4s1Eaz9z0ofY4VI0TV2Wd4dwz1NeJC7/ xQit94xTxL5eiGI8KQhR8zWLAjKsLSIbFr/C4Y/CZNB0voo0Kz1bBPo9Z/T1spsC plJ70tkzQmaWqH4YnREO2gnt72wB6q83OjmdHVHuymFGtE7A8ltjE0FGCU1au5Vc 8LYzXvTkhbMY01iDlsO7o+4WtMmNbzmk6bcfLsagYtcdZgegy0iGoa5s+KBp3Cmk hYAlN2YHzioBS1JKbbDfQKQbfzJgZ1a4srYYaiFysWpyI4Uj5kHWdJW46tCRytQ= =TziA -----END PGP SIGNATURE-----
Siddhesh Poyarekar <siddhesh@redhat.com> writes: > diff --git a/resolv/res_query.c b/resolv/res_query.c > index a9db837..4e6612c 100644 > --- a/resolv/res_query.c > +++ b/resolv/res_query.c > @@ -382,7 +382,9 @@ __libc_res_nsearch(res_state statp, > answer, anslen, answerp, > answerp2, nanswerp2, resplen2, > answerp2_malloced); > - if (ret > 0 || trailing_dot) > + if (ret > 0 || trailing_dot > + /* If the second response is valid then we use that. */ > + || (ret == 0 && answerp2 != NULL && resplen2 > 0)) That doesn't make sense, resplen2 is a pointer. I'm surprised that the compiler does not warn about that. I think that answerp2 != NULL should be resplen2 != NULL. > @@ -422,7 +424,8 @@ __libc_res_nsearch(res_state statp, > answer, anslen, answerp, > answerp2, nanswerp2, > resplen2, answerp2_malloced); > - if (ret > 0) > + if (ret > 0 || (ret == 0 && answerp2 != NULL > + && resplen2 > 0)) Likewise. Andreas.
There is also a third place in res_query.c that should probably get the same change: /* * If the query has not already been tried as is then try it * unless RES_NOTLDQUERY is set and there were no dots. */ if ((dots || !searched || (statp->options & RES_NOTLDQUERY) == 0) && !(tried_as_is || root_on_list)) { ret = __libc_res_nquerydomain(statp, name, NULL, class, type, answer, anslen, answerp, answerp2, nanswerp2, resplen2, answerp2_malloced); if (ret > 0) return (ret); } Andreas.
diff --git a/resolv/res_query.c b/resolv/res_query.c index a9db837..4e6612c 100644 --- a/resolv/res_query.c +++ b/resolv/res_query.c @@ -382,7 +382,9 @@ __libc_res_nsearch(res_state statp, answer, anslen, answerp, answerp2, nanswerp2, resplen2, answerp2_malloced); - if (ret > 0 || trailing_dot) + if (ret > 0 || trailing_dot + /* If the second response is valid then we use that. */ + || (ret == 0 && answerp2 != NULL && resplen2 > 0)) return (ret); saved_herrno = h_errno; tried_as_is++; @@ -422,7 +424,8 @@ __libc_res_nsearch(res_state statp, answer, anslen, answerp, answerp2, nanswerp2, resplen2, answerp2_malloced); - if (ret > 0) + if (ret > 0 || (ret == 0 && answerp2 != NULL + && resplen2 > 0)) return (ret); if (answerp && *answerp != answer) { diff --git a/resolv/res_send.c b/resolv/res_send.c index 60743df..3273d55 100644 --- a/resolv/res_send.c +++ b/resolv/res_send.c @@ -1351,6 +1351,7 @@ send_dg(res_state statp, (*thisresplenp > *thisanssizp) ? *thisanssizp : *thisresplenp); + next_ns: if (recvresp1 || (buf2 != NULL && recvresp2)) { *resplen2 = 0; return resplen; @@ -1368,7 +1369,6 @@ send_dg(res_state statp, goto wait; } - next_ns: __res_iclose(statp, false); /* don't retry if called from dig */ if (!statp->pfcode)