| Message ID | c09960595ff4fca592df9ec7e9203a99b3c91fa5.1718345824.git.fweimer@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Some DNS stub resolver fixes/enhancements | expand |
* Florian Weimer: > +{ > + switch (ctx->server_index) > + { > + case 0: > + /* First server times out. */ > + struct resolv_response_flags flags = {.rcode = rcode}; > + resolv_response_init (b, flags); > + break; This needs more braces for GCC 8 compatibility. Fixed locally. Thanks, Florian
Florian Weimer <fweimer@redhat.com> writes: > --- > resolv/Makefile | 3 + > resolv/res_send.c | 29 +++++--- > resolv/tst-resolv-short-response.c | 112 +++++++++++++++++++++++++++++ > 3 files changed, 134 insertions(+), 10 deletions(-) > create mode 100644 resolv/tst-resolv-short-response.c > > diff --git a/resolv/Makefile b/resolv/Makefile > index 5f44f5896b..d927e337d9 100644 > --- a/resolv/Makefile > +++ b/resolv/Makefile > @@ -106,6 +106,7 @@ tests += \ > tst-resolv-nondecimal \ > tst-resolv-res_init-multi \ > tst-resolv-search \ > + tst-resolv-short-response \ > tst-resolv-trailing \ > > # This test calls __res_context_send directly, which is not exported > @@ -299,6 +300,8 @@ $(objpfx)tst-resolv-nondecimal: $(objpfx)libresolv.so $(shared-thread-library) > $(objpfx)tst-resolv-qtypes: $(objpfx)libresolv.so $(shared-thread-library) > $(objpfx)tst-resolv-rotate: $(objpfx)libresolv.so $(shared-thread-library) > $(objpfx)tst-resolv-search: $(objpfx)libresolv.so $(shared-thread-library) > +$(objpfx)tst-resolv-short-response: $(objpfx)libresolv.so \ > + $(shared-thread-library) > $(objpfx)tst-resolv-trailing: $(objpfx)libresolv.so $(shared-thread-library) > $(objpfx)tst-resolv-threads: $(objpfx)libresolv.so $(shared-thread-library) > $(objpfx)tst-resolv-txnid-collision: $(objpfx)libresolv.a \ > diff --git a/resolv/res_send.c b/resolv/res_send.c > index ea7cf192b2..572e72c32f 100644 > --- a/resolv/res_send.c > +++ b/resolv/res_send.c > @@ -1199,19 +1199,30 @@ send_dg(res_state statp, > } > > /* Check for the correct header layout and a matching > - question. */ > + question. Some recursive resolvers send REFUSED > + without copying back the question section > + (producing a response that is only HFIXEDSZ bytes > + long). Skip query matching in this case. */ > + bool thisansp_error = (anhp->rcode == SERVFAIL || > + anhp->rcode == NOTIMP || > + anhp->rcode == REFUSED); > + bool skip_query_match = (*thisresplenp == HFIXEDSZ > + && ntohs (anhp->qdcount) == 0 > + && thisansp_error); > int matching_query = 0; /* Default to no matching query. */ > if (!recvresp1 > && anhp->id == hp->id > - && __libc_res_queriesmatch (buf, buf + buflen, > - *thisansp, > - *thisansp + *thisanssizp)) > + && (skip_query_match > + || __libc_res_queriesmatch (buf, buf + buflen, > + *thisansp, > + *thisansp + *thisanssizp))) > matching_query = 1; > if (!recvresp2 > && anhp->id == hp2->id > - && __libc_res_queriesmatch (buf2, buf2 + buflen2, > - *thisansp, > - *thisansp + *thisanssizp)) > + && (skip_query_match > + || __libc_res_queriesmatch (buf2, buf2 + buflen2, > + *thisansp, > + *thisansp + *thisanssizp))) > matching_query = 2; > if (matching_query == 0) > /* Spurious UDP packet. Drop it and continue > @@ -1221,9 +1232,7 @@ send_dg(res_state statp, > goto wait; > } > > - if (anhp->rcode == SERVFAIL || > - anhp->rcode == NOTIMP || > - anhp->rcode == REFUSED) { > + if (thisansp_error) { > next_ns: > if (recvresp1 || (buf2 != NULL && recvresp2)) { > *resplen2 = 0; > diff --git a/resolv/tst-resolv-short-response.c b/resolv/tst-resolv-short-response.c > new file mode 100644 > index 0000000000..0647de0704 > --- /dev/null > +++ b/resolv/tst-resolv-short-response.c > @@ -0,0 +1,112 @@ > +/* Test for spurious timeouts with short 12-byte responses (bug 31890). > + Copyright (C) 2024 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <resolv.h> > +#include <support/check.h> > +#include <support/resolv_test.h> > +#include <support/check_nss.h> > + > +/* The rcode in the initial response. */ > +static volatile int rcode; > + > +static void > +response (const struct resolv_response_context *ctx, > + struct resolv_response_builder *b, > + const char *qname, uint16_t qclass, uint16_t qtype) > +{ > + switch (ctx->server_index) > + { > + case 0: > + /* First server times out. */ > + struct resolv_response_flags flags = {.rcode = rcode}; > + resolv_response_init (b, flags); > + break; > + case 1: > + /* Second server sends reply. */ > + resolv_response_init (b, (struct resolv_response_flags) {}); > + resolv_response_add_question (b, qname, qclass, qtype); > + resolv_response_section (b, ns_s_an); > + resolv_response_open_record (b, qname, qclass, qtype, 0); > + switch (qtype) > + { > + case T_A: > + { > + char ipv4[4] = {192, 0, 2, 17}; > + resolv_response_add_data (b, &ipv4, sizeof (ipv4)); > + } > + break; > + case T_AAAA: > + { > + char ipv6[16] > + = {0x20, 0x01, 0xd, 0xb8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}; > + resolv_response_add_data (b, &ipv6, sizeof (ipv6)); > + } > + break; > + default: > + FAIL_EXIT1 ("unexpected TYPE%d query", qtype); > + } > + resolv_response_close_record (b); > + break; > + default: > + FAIL_EXIT1 ("unexpected query to server %d", ctx->server_index); > + } > +} > + > +static void > +check_one (void) > +{ > + > + /* The buggy 1-second query timeout results in 30 seconds of delay, > + which triggers are test timeout failure. */ "which triggers a test timeout failure"? > [...]
* Sam James: >> +static void >> +check_one (void) >> +{ >> + >> + /* The buggy 1-second query timeout results in 30 seconds of delay, >> + which triggers are test timeout failure. */ > > "which triggers a test timeout failure"? Thanks, fixed locally. Florian
diff --git a/resolv/Makefile b/resolv/Makefile index 5f44f5896b..d927e337d9 100644 --- a/resolv/Makefile +++ b/resolv/Makefile @@ -106,6 +106,7 @@ tests += \ tst-resolv-nondecimal \ tst-resolv-res_init-multi \ tst-resolv-search \ + tst-resolv-short-response \ tst-resolv-trailing \ # This test calls __res_context_send directly, which is not exported @@ -299,6 +300,8 @@ $(objpfx)tst-resolv-nondecimal: $(objpfx)libresolv.so $(shared-thread-library) $(objpfx)tst-resolv-qtypes: $(objpfx)libresolv.so $(shared-thread-library) $(objpfx)tst-resolv-rotate: $(objpfx)libresolv.so $(shared-thread-library) $(objpfx)tst-resolv-search: $(objpfx)libresolv.so $(shared-thread-library) +$(objpfx)tst-resolv-short-response: $(objpfx)libresolv.so \ + $(shared-thread-library) $(objpfx)tst-resolv-trailing: $(objpfx)libresolv.so $(shared-thread-library) $(objpfx)tst-resolv-threads: $(objpfx)libresolv.so $(shared-thread-library) $(objpfx)tst-resolv-txnid-collision: $(objpfx)libresolv.a \ diff --git a/resolv/res_send.c b/resolv/res_send.c index ea7cf192b2..572e72c32f 100644 --- a/resolv/res_send.c +++ b/resolv/res_send.c @@ -1199,19 +1199,30 @@ send_dg(res_state statp, } /* Check for the correct header layout and a matching - question. */ + question. Some recursive resolvers send REFUSED + without copying back the question section + (producing a response that is only HFIXEDSZ bytes + long). Skip query matching in this case. */ + bool thisansp_error = (anhp->rcode == SERVFAIL || + anhp->rcode == NOTIMP || + anhp->rcode == REFUSED); + bool skip_query_match = (*thisresplenp == HFIXEDSZ + && ntohs (anhp->qdcount) == 0 + && thisansp_error); int matching_query = 0; /* Default to no matching query. */ if (!recvresp1 && anhp->id == hp->id - && __libc_res_queriesmatch (buf, buf + buflen, - *thisansp, - *thisansp + *thisanssizp)) + && (skip_query_match + || __libc_res_queriesmatch (buf, buf + buflen, + *thisansp, + *thisansp + *thisanssizp))) matching_query = 1; if (!recvresp2 && anhp->id == hp2->id - && __libc_res_queriesmatch (buf2, buf2 + buflen2, - *thisansp, - *thisansp + *thisanssizp)) + && (skip_query_match + || __libc_res_queriesmatch (buf2, buf2 + buflen2, + *thisansp, + *thisansp + *thisanssizp))) matching_query = 2; if (matching_query == 0) /* Spurious UDP packet. Drop it and continue @@ -1221,9 +1232,7 @@ send_dg(res_state statp, goto wait; } - if (anhp->rcode == SERVFAIL || - anhp->rcode == NOTIMP || - anhp->rcode == REFUSED) { + if (thisansp_error) { next_ns: if (recvresp1 || (buf2 != NULL && recvresp2)) { *resplen2 = 0; diff --git a/resolv/tst-resolv-short-response.c b/resolv/tst-resolv-short-response.c new file mode 100644 index 0000000000..0647de0704 --- /dev/null +++ b/resolv/tst-resolv-short-response.c @@ -0,0 +1,112 @@ +/* Test for spurious timeouts with short 12-byte responses (bug 31890). + Copyright (C) 2024 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <resolv.h> +#include <support/check.h> +#include <support/resolv_test.h> +#include <support/check_nss.h> + +/* The rcode in the initial response. */ +static volatile int rcode; + +static void +response (const struct resolv_response_context *ctx, + struct resolv_response_builder *b, + const char *qname, uint16_t qclass, uint16_t qtype) +{ + switch (ctx->server_index) + { + case 0: + /* First server times out. */ + struct resolv_response_flags flags = {.rcode = rcode}; + resolv_response_init (b, flags); + break; + case 1: + /* Second server sends reply. */ + resolv_response_init (b, (struct resolv_response_flags) {}); + resolv_response_add_question (b, qname, qclass, qtype); + resolv_response_section (b, ns_s_an); + resolv_response_open_record (b, qname, qclass, qtype, 0); + switch (qtype) + { + case T_A: + { + char ipv4[4] = {192, 0, 2, 17}; + resolv_response_add_data (b, &ipv4, sizeof (ipv4)); + } + break; + case T_AAAA: + { + char ipv6[16] + = {0x20, 0x01, 0xd, 0xb8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}; + resolv_response_add_data (b, &ipv6, sizeof (ipv6)); + } + break; + default: + FAIL_EXIT1 ("unexpected TYPE%d query", qtype); + } + resolv_response_close_record (b); + break; + default: + FAIL_EXIT1 ("unexpected query to server %d", ctx->server_index); + } +} + +static void +check_one (void) +{ + + /* The buggy 1-second query timeout results in 30 seconds of delay, + which triggers are test timeout failure. */ + for (int i = 0; i < 10; ++i) + { + check_hostent ("www.example", gethostbyname ("www.example"), + "name: www.example\n" + "address: 192.0.2.17\n"); + check_hostent ("www.example", gethostbyname2 ("www.example", AF_INET6), + "name: www.example\n" + "address: 2001:db8::1\n"); + } +} + +static int +do_test (void) +{ + struct resolv_test *aux = resolv_test_start + ((struct resolv_redirect_config) + { + .response_callback = response, + }); + + _res.options |= RES_SNGLKUP; + + rcode = 2; /* SERVFAIL. */ + check_one (); + + rcode = 4; /* NOTIMP. */ + check_one (); + + rcode = 5; /* REFUSED. */ + check_one (); + + resolv_test_end (aux); + + return 0; +} + +#include <support/test-driver.c>