[1/4] resolv: Allow short error responses to match any query (bug 31890)
Checks
| Context |
Check |
Description |
| redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Test passed
|
Commit Message
---
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
Comments
* 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
Florian Weimer <fweimer@redhat.com> writes:
> + tst-resolv-short-response \
Ok.
> +$(objpfx)tst-resolv-short-response: $(objpfx)libresolv.so \
> + $(shared-thread-library)
Ok.
> 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);
thisansp_error is set if we get the "right" type of error.
skip_query_match is set if the length and count indicate an empty
answer. Ok.
> 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;
So if the answer is short, we don't try parsing it. Ok.
> 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;
Likewise here, ok.
>
> - if (anhp->rcode == SERVFAIL ||
> - anhp->rcode == NOTIMP ||
> - anhp->rcode == REFUSED) {
> + if (thisansp_error) {
Ok.
> diff --git a/resolv/tst-resolv-short-response.c b/resolv/tst-resolv-short-response.c
> +/* 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>
Ok.
> +/* The rcode in the initial response. */
> +static volatile int rcode;
Ok.
> +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;
Ok.
> + 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;
Ok.
> + default:
> + FAIL_EXIT1 ("unexpected query to server %d", ctx->server_index);
> + }
> +}
Ok.
> +static void
> +check_one (void)
> +{
> +
> + /* The buggy 1-second query timeout results in 30 seconds of delay,
> + which triggers are test timeout failure. */
Will this delay be enough on small embedded boards where we set
TIMEOUTFACTOR to 20 or more? It might be worth adding an explicit set
of time() calls.
I don't think we should rely on the test harness itself to check for
time-sensitive results.
> + 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");
> + }
> +}
Ok.
> +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>
Ok.
* DJ Delorie:
>> +static void
>> +check_one (void)
>> +{
>> +
>> + /* The buggy 1-second query timeout results in 30 seconds of delay,
>> + which triggers are test timeout failure. */
>
> Will this delay be enough on small embedded boards where we set
> TIMEOUTFACTOR to 20 or more? It might be worth adding an explicit set
> of time() calls.
>
> I don't think we should rely on the test harness itself to check for
> time-sensitive results.
The tight time-based checks may fire unconditionally on small or loaded
systems, so that's not great, either.
The issue is generic, so I think it's okay if the reproduces the bug
only with a default TIMEOUTFACTOR. I think a false negative here is
better than a false positive.
Thanks,
Florian
Florian Weimer <fweimer@redhat.com> writes:
>>> +static void
>>> +check_one (void)
>>> +{
>>> +
>>> + /* The buggy 1-second query timeout results in 30 seconds of delay,
>>> + which triggers are test timeout failure. */
>>
>> Will this delay be enough on small embedded boards where we set
>> TIMEOUTFACTOR to 20 or more? It might be worth adding an explicit set
>> of time() calls.
>>
>> I don't think we should rely on the test harness itself to check for
>> time-sensitive results.
>
> The tight time-based checks may fire unconditionally on small or loaded
> systems, so that's not great, either.
>
> The issue is generic, so I think it's okay if the reproduces the bug
> only with a default TIMEOUTFACTOR. I think a false negative here is
> better than a false positive.
I would think the difference between "immediately" and "30 seconds"
should be easy enough to get right with time(), but I'm not going to
argue it.
Reviewed-by: DJ Delorie <dj@redhat.com>
@@ -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 \
@@ -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;
new file mode 100644
@@ -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>