* [PATCH 0/4] Some DNS stub resolver fixes/enhancements
@ 2024-06-14 6:20 Florian Weimer
2024-06-14 6:20 ` [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890) Florian Weimer
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Florian Weimer @ 2024-06-14 6:20 UTC (permalink / raw)
To: libc-alpha
This series started out with a fix for bug 31890, but the new tests
found other bugs.
The “-” option processing in this context is desirable so that it's
possible to override strict-error in /etc/resolv.conf with
RES_OPTIONS=-strict-error on the command line.
I ran the testsuite with the default changed to RES_STRICTERR (in both
places), and there were no new failures, except for the res_init
reporting of the default.
Tested on x86_64-linux-gnu.
Thanks,
Florian
Florian Weimer (4):
resolv: Allow short error responses to match any query (bug 31890)
resolv: Do not wait for non-existing second DNS response after error
(bug 30081)
resolv: Support clearing option flags with a “-” prefix (bug 14799)
resolv: Implement strict-error stub resolver option (bug 27929)
NEWS | 16 +++
resolv/Makefile | 6 ++
resolv/res_init.c | 29 +++---
resolv/res_send.c | 74 +++++++++-----
resolv/resolv.h | 1 +
resolv/tst-resolv-res_init-skeleton.c | 20 ++++
resolv/tst-resolv-semi-failure.c | 142 ++++++++++++++++++++++++++
resolv/tst-resolv-short-response.c | 124 ++++++++++++++++++++++
8 files changed, 375 insertions(+), 37 deletions(-)
create mode 100644 resolv/tst-resolv-semi-failure.c
create mode 100644 resolv/tst-resolv-short-response.c
base-commit: 868ab8923a2ec977faafec97ecafac0c3159c1b2
--
2.45.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890)
2024-06-14 6:20 [PATCH 0/4] Some DNS stub resolver fixes/enhancements Florian Weimer
@ 2024-06-14 6:20 ` Florian Weimer
2024-06-14 7:43 ` Florian Weimer
` (2 more replies)
2024-06-14 6:20 ` [PATCH 2/4] resolv: Do not wait for non-existing second DNS response after error (bug 30081) Florian Weimer
` (2 subsequent siblings)
3 siblings, 3 replies; 14+ messages in thread
From: Florian Weimer @ 2024-06-14 6:20 UTC (permalink / raw)
To: libc-alpha
---
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. */
+ 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>
--
2.45.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] resolv: Do not wait for non-existing second DNS response after error (bug 30081)
2024-06-14 6:20 [PATCH 0/4] Some DNS stub resolver fixes/enhancements Florian Weimer
2024-06-14 6:20 ` [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890) Florian Weimer
@ 2024-06-14 6:20 ` Florian Weimer
2024-06-20 8:37 ` Andreas Schwab
2024-06-20 20:41 ` DJ Delorie
2024-06-14 6:20 ` [PATCH 3/4] resolv: Support clearing option flags with a “-” prefix (bug 14799) Florian Weimer
2024-06-14 6:21 ` [PATCH 4/4] resolv: Implement strict-error stub resolver option (bug 27929) Florian Weimer
3 siblings, 2 replies; 14+ messages in thread
From: Florian Weimer @ 2024-06-14 6:20 UTC (permalink / raw)
To: libc-alpha
In single-request mode, there is no second response after an error
because the second query has not been sent yet. Waiting for it
introduces an unnecessary timeout.
---
resolv/Makefile | 3 +
resolv/res_send.c | 2 +-
resolv/tst-resolv-semi-failure.c | 133 +++++++++++++++++++++++++++++
resolv/tst-resolv-short-response.c | 12 +++
4 files changed, 149 insertions(+), 1 deletion(-)
create mode 100644 resolv/tst-resolv-semi-failure.c
diff --git a/resolv/Makefile b/resolv/Makefile
index d927e337d9..abff7fc007 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-semi-failure \
tst-resolv-short-response \
tst-resolv-trailing \
@@ -300,6 +301,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-semi-failure: $(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)
diff --git a/resolv/res_send.c b/resolv/res_send.c
index 572e72c32f..9c77613f37 100644
--- a/resolv/res_send.c
+++ b/resolv/res_send.c
@@ -1238,7 +1238,7 @@ send_dg(res_state statp,
*resplen2 = 0;
return resplen;
}
- if (buf2 != NULL)
+ if (buf2 != NULL && !single_request)
{
/* No data from the first reply. */
resplen = 0;
diff --git a/resolv/tst-resolv-semi-failure.c b/resolv/tst-resolv-semi-failure.c
new file mode 100644
index 0000000000..aa9798b5a7
--- /dev/null
+++ b/resolv/tst-resolv-semi-failure.c
@@ -0,0 +1,133 @@
+/* Test parallel failure/success responses (bug 30081).
+ 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;
+
+/* Whether to fail the initial A query (!fail_aaaa) or the initial
+ AAAA query (fail_aaaa). */
+static volatile bool fail_aaaa;
+
+static void
+response (const struct resolv_response_context *ctx,
+ struct resolv_response_builder *b,
+ const char *qname, uint16_t qclass, uint16_t qtype)
+{
+ /* Handle the failing query. */
+ if ((fail_aaaa && qtype == T_AAAA) && ctx->server_index == 0)
+ {
+ struct resolv_response_flags flags = {.rcode = rcode};
+ resolv_response_init (b, flags);
+ return;
+ }
+
+ /* Otherwise produce a response. */
+ 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);
+}
+
+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 < 30; ++i)
+ {
+ static const struct addrinfo hints =
+ {
+ .ai_family = AF_UNSPEC,
+ .ai_socktype = SOCK_STREAM,
+ };
+ struct addrinfo *ai;
+ int ret = getaddrinfo ("www.example", "80", &hints, &ai);
+ const char *expected;
+ if (ret == 0 && ai->ai_next != NULL)
+ expected = ("address: STREAM/TCP 192.0.2.17 80\n"
+ "address: STREAM/TCP 2001:db8::1 80\n");
+ else
+ /* Only one response because the AAAA lookup failure is
+ treated as an ignoreable error. */
+ expected = "address: STREAM/TCP 192.0.2.17 80\n";
+ check_addrinfo ("www.example", ai, ret, expected);
+ if (ret == 0)
+ freeaddrinfo (ai);
+ }
+}
+
+static int
+do_test (void)
+{
+ for (int do_single_lookup = 0; do_single_lookup < 2; ++do_single_lookup)
+ {
+ struct resolv_test *aux = resolv_test_start
+ ((struct resolv_redirect_config)
+ {
+ .response_callback = response,
+ });
+
+ if (do_single_lookup)
+ _res.options |= RES_SNGLKUP;
+
+ for (int do_fail_aaaa = 0; do_fail_aaaa < 2; ++do_fail_aaaa)
+ {
+ fail_aaaa = do_fail_aaaa;
+
+ 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>
diff --git a/resolv/tst-resolv-short-response.c b/resolv/tst-resolv-short-response.c
index 0647de0704..a6ea456e46 100644
--- a/resolv/tst-resolv-short-response.c
+++ b/resolv/tst-resolv-short-response.c
@@ -81,6 +81,18 @@ check_one (void)
check_hostent ("www.example", gethostbyname2 ("www.example", AF_INET6),
"name: www.example\n"
"address: 2001:db8::1\n");
+ static const struct addrinfo hints =
+ {
+ .ai_family = AF_UNSPEC,
+ .ai_socktype = SOCK_STREAM,
+ };
+ struct addrinfo *ai;
+ int ret = getaddrinfo ("www.example", "80", &hints, &ai);
+ check_addrinfo ("www.example", ai, ret,
+ "address: STREAM/TCP 192.0.2.17 80\n"
+ "address: STREAM/TCP 2001:db8::1 80\n");
+ if (ret == 0)
+ freeaddrinfo (ai);
}
}
--
2.45.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] resolv: Support clearing option flags with a “-” prefix (bug 14799)
2024-06-14 6:20 [PATCH 0/4] Some DNS stub resolver fixes/enhancements Florian Weimer
2024-06-14 6:20 ` [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890) Florian Weimer
2024-06-14 6:20 ` [PATCH 2/4] resolv: Do not wait for non-existing second DNS response after error (bug 30081) Florian Weimer
@ 2024-06-14 6:20 ` Florian Weimer
2024-06-20 21:06 ` DJ Delorie
2024-06-14 6:21 ` [PATCH 4/4] resolv: Implement strict-error stub resolver option (bug 27929) Florian Weimer
3 siblings, 1 reply; 14+ messages in thread
From: Florian Weimer @ 2024-06-14 6:20 UTC (permalink / raw)
To: libc-alpha
I think using a “-” prefix is less confusing than introducing
double-negation construct (“no-no-tld-query”).
---
NEWS | 6 ++++++
resolv/res_init.c | 28 ++++++++++++++-------------
resolv/tst-resolv-res_init-skeleton.c | 10 ++++++++++
3 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/NEWS b/NEWS
index 20e263f581..495bbd5cbc 100644
--- a/NEWS
+++ b/NEWS
@@ -36,6 +36,12 @@ Major new features:
* On Linux, update epoll header to include epoll ioctl definitions and
related structure added in Linux kernel 6.9.
+* In /etc/resolv.conf and the RES_OPTIONS, option flags can now be
+ prefixed with “-” to clear previously set flags. For example, if
+ /etc/resolv.conf contains “options no-aaaa”, a process running with
+ the RES_OPTIONS=-no-aaaa environment variable performs AAAA DNS
+ queries when the glibc DNS stub resolver is used.
+
Deprecated and removed features, and other changes affecting compatibility:
* Architectures which use a 32-bit seconds-since-epoch field in struct
diff --git a/resolv/res_init.c b/resolv/res_init.c
index 263263d474..243532b3ad 100644
--- a/resolv/res_init.c
+++ b/resolv/res_init.c
@@ -682,27 +682,29 @@ res_setoptions (struct resolv_conf_parser *parser, const char *options)
{
char str[22];
uint8_t len;
- uint8_t clear;
unsigned long int flag;
} options[] = {
#define STRnLEN(str) str, sizeof (str) - 1
- { STRnLEN ("rotate"), 0, RES_ROTATE },
- { STRnLEN ("edns0"), 0, RES_USE_EDNS0 },
- { STRnLEN ("single-request-reopen"), 0, RES_SNGLKUPREOP },
- { STRnLEN ("single-request"), 0, RES_SNGLKUP },
- { STRnLEN ("no_tld_query"), 0, RES_NOTLDQUERY },
- { STRnLEN ("no-tld-query"), 0, RES_NOTLDQUERY },
- { STRnLEN ("no-reload"), 0, RES_NORELOAD },
- { STRnLEN ("use-vc"), 0, RES_USEVC },
- { STRnLEN ("trust-ad"), 0, RES_TRUSTAD },
- { STRnLEN ("no-aaaa"), 0, RES_NOAAAA },
+ { STRnLEN ("rotate"), RES_ROTATE },
+ { STRnLEN ("edns0"), RES_USE_EDNS0 },
+ { STRnLEN ("single-request-reopen"), RES_SNGLKUPREOP },
+ { STRnLEN ("single-request"), RES_SNGLKUP },
+ { STRnLEN ("no_tld_query"), RES_NOTLDQUERY },
+ { STRnLEN ("no-tld-query"), RES_NOTLDQUERY },
+ { STRnLEN ("no-reload"), RES_NORELOAD },
+ { STRnLEN ("use-vc"), RES_USEVC },
+ { STRnLEN ("trust-ad"), RES_TRUSTAD },
+ { STRnLEN ("no-aaaa"), RES_NOAAAA },
};
#define noptions (sizeof (options) / sizeof (options[0]))
+ bool negate_option = *cp == '-';
+ if (negate_option)
+ ++cp;
for (int i = 0; i < noptions; ++i)
if (strncmp (cp, options[i].str, options[i].len) == 0)
{
- if (options[i].clear)
- parser->template.options &= options[i].flag;
+ if (negate_option)
+ parser->template.options &= ~options[i].flag;
else
parser->template.options |= options[i].flag;
break;
diff --git a/resolv/tst-resolv-res_init-skeleton.c b/resolv/tst-resolv-res_init-skeleton.c
index 6bef62cde2..d3a19eb305 100644
--- a/resolv/tst-resolv-res_init-skeleton.c
+++ b/resolv/tst-resolv-res_init-skeleton.c
@@ -679,6 +679,16 @@ struct test_case test_cases[] =
"; nameserver[0]: [192.0.2.1]:53\n",
.res_options = "attempts:5 ndots:3 edns0 ",
},
+ {.name = "RES_OPTIONS can clear flags",
+ .conf = "options ndots:2 use-vc no-aaaa edns0\n"
+ "nameserver 192.0.2.1\n",
+ .expected = "options ndots:3 use-vc\n"
+ "search example.com\n"
+ "; search[0]: example.com\n"
+ "nameserver 192.0.2.1\n"
+ "; nameserver[0]: [192.0.2.1]:53\n",
+ .res_options = "ndots:3 -edns0 -no-aaaa",
+ },
{.name = "many search list entries (bug 19569)",
.conf = "nameserver 192.0.2.1\n"
"search corp.example.com support.example.com"
--
2.45.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/4] resolv: Implement strict-error stub resolver option (bug 27929)
2024-06-14 6:20 [PATCH 0/4] Some DNS stub resolver fixes/enhancements Florian Weimer
` (2 preceding siblings ...)
2024-06-14 6:20 ` [PATCH 3/4] resolv: Support clearing option flags with a “-” prefix (bug 14799) Florian Weimer
@ 2024-06-14 6:21 ` Florian Weimer
2024-06-20 21:24 ` DJ Delorie
3 siblings, 1 reply; 14+ messages in thread
From: Florian Weimer @ 2024-06-14 6:21 UTC (permalink / raw)
To: libc-alpha
For now, do not enable this mode by default due to the potential
impact on compatibility with existing deployments.
---
NEWS | 10 +++++
resolv/res_init.c | 1 +
resolv/res_send.c | 45 ++++++++++++++++-------
resolv/resolv.h | 1 +
resolv/tst-resolv-res_init-skeleton.c | 10 +++++
resolv/tst-resolv-semi-failure.c | 53 ++++++++++++++++-----------
6 files changed, 84 insertions(+), 36 deletions(-)
diff --git a/NEWS b/NEWS
index 495bbd5cbc..2149da63c4 100644
--- a/NEWS
+++ b/NEWS
@@ -42,6 +42,16 @@ Major new features:
the RES_OPTIONS=-no-aaaa environment variable performs AAAA DNS
queries when the glibc DNS stub resolver is used.
+* The DNS stub resolver now supports the strict-error option. If
+ activated, getaddrinfo for the AF_UNSPEC address family (with dual
+ A/AAAA DNS lookups) attemps to obtain a A/AAAA response pair from
+ another DNS server if one of the responses indicates failure. Without
+ the strict-error option, getaddrinfo returns the A record data it has
+ obtained even if the AAAA query failed. The new strict error mode is
+ incompatible with some DNS environments which do not follow the RFCs,
+ which is why this mode is not enabled by default. A future version
+ of the library may turn it on by default, however.
+
Deprecated and removed features, and other changes affecting compatibility:
* Architectures which use a 32-bit seconds-since-epoch field in struct
diff --git a/resolv/res_init.c b/resolv/res_init.c
index 243532b3ad..b838dc7064 100644
--- a/resolv/res_init.c
+++ b/resolv/res_init.c
@@ -695,6 +695,7 @@ res_setoptions (struct resolv_conf_parser *parser, const char *options)
{ STRnLEN ("use-vc"), RES_USEVC },
{ STRnLEN ("trust-ad"), RES_TRUSTAD },
{ STRnLEN ("no-aaaa"), RES_NOAAAA },
+ { STRnLEN ("strict-error"), RES_STRICTERR },
};
#define noptions (sizeof (options) / sizeof (options[0]))
bool negate_option = *cp == '-';
diff --git a/resolv/res_send.c b/resolv/res_send.c
index 9c77613f37..9a284ed44a 100644
--- a/resolv/res_send.c
+++ b/resolv/res_send.c
@@ -1234,21 +1234,38 @@ send_dg(res_state statp,
if (thisansp_error) {
next_ns:
- if (recvresp1 || (buf2 != NULL && recvresp2)) {
- *resplen2 = 0;
- return resplen;
- }
- if (buf2 != NULL && !single_request)
+ /* Outside of strict-error mode, use the first
+ response even if the second response is an
+ error. This allows parallel resolution to
+ succeed even if the recursive resolver
+ always answers with SERVFAIL for AAAA
+ queries (which still happens in practice
+ unfortunately).
+
+ In strict-error mode, always switch to the
+ next server and try to get a response from
+ there. */
+ if ((statp->options & RES_STRICTERR) == 0)
{
- /* No data from the first reply. */
- resplen = 0;
- /* We are waiting for a possible second reply. */
- if (matching_query == 1)
- recvresp1 = 1;
- else
- recvresp2 = 1;
-
- goto wait;
+ if (recvresp1 || (buf2 != NULL && recvresp2))
+ {
+ *resplen2 = 0;
+ return resplen;
+ }
+
+ if (buf2 != NULL && !single_request)
+ {
+ /* No data from the first reply. */
+ resplen = 0;
+ /* We are waiting for a possible
+ second reply. */
+ if (matching_query == 1)
+ recvresp1 = 1;
+ else
+ recvresp2 = 1;
+
+ goto wait;
+ }
}
/* don't retry if called from dig */
diff --git a/resolv/resolv.h b/resolv/resolv.h
index f40d6c58ce..b8a0f66a5f 100644
--- a/resolv/resolv.h
+++ b/resolv/resolv.h
@@ -133,6 +133,7 @@ struct res_sym {
#define RES_NORELOAD 0x02000000 /* No automatic configuration reload. */
#define RES_TRUSTAD 0x04000000 /* Request AD bit, keep it in responses. */
#define RES_NOAAAA 0x08000000 /* Suppress AAAA queries. */
+#define RES_STRICTERR 0x10000000 /* Report more DNS errors as errors. */
#define RES_DEFAULT (RES_RECURSE|RES_DEFNAMES|RES_DNSRCH)
diff --git a/resolv/tst-resolv-res_init-skeleton.c b/resolv/tst-resolv-res_init-skeleton.c
index d3a19eb305..e41bcebd9d 100644
--- a/resolv/tst-resolv-res_init-skeleton.c
+++ b/resolv/tst-resolv-res_init-skeleton.c
@@ -129,6 +129,7 @@ print_resp (FILE *fp, res_state resp)
print_option_flag (fp, &options, RES_NORELOAD, "no-reload");
print_option_flag (fp, &options, RES_TRUSTAD, "trust-ad");
print_option_flag (fp, &options, RES_NOAAAA, "no-aaaa");
+ print_option_flag (fp, &options, RES_STRICTERR, "strict-error");
fputc ('\n', fp);
if (options != 0)
fprintf (fp, "; error: unresolved option bits: 0x%x\n", options);
@@ -741,6 +742,15 @@ struct test_case test_cases[] =
"nameserver 192.0.2.1\n"
"; nameserver[0]: [192.0.2.1]:53\n"
},
+ {.name = "strict-error flag",
+ .conf = "options strict-error\n"
+ "nameserver 192.0.2.1\n",
+ .expected = "options strict-error\n"
+ "search example.com\n"
+ "; search[0]: example.com\n"
+ "nameserver 192.0.2.1\n"
+ "; nameserver[0]: [192.0.2.1]:53\n"
+ },
{ NULL }
};
diff --git a/resolv/tst-resolv-semi-failure.c b/resolv/tst-resolv-semi-failure.c
index aa9798b5a7..b7681210f4 100644
--- a/resolv/tst-resolv-semi-failure.c
+++ b/resolv/tst-resolv-semi-failure.c
@@ -67,6 +67,9 @@ response (const struct resolv_response_context *ctx,
resolv_response_close_record (b);
}
+/* Set to 1 if strict error checking is enabled. */
+static int do_strict_error;
+
static void
check_one (void)
{
@@ -83,7 +86,10 @@ check_one (void)
struct addrinfo *ai;
int ret = getaddrinfo ("www.example", "80", &hints, &ai);
const char *expected;
- if (ret == 0 && ai->ai_next != NULL)
+ /* In strict-error mode, a switch to the second name server
+ happens, and both responses are received, so a single
+ response is a bug. */
+ if (do_strict_error || (ret == 0 && ai->ai_next != NULL))
expected = ("address: STREAM/TCP 192.0.2.17 80\n"
"address: STREAM/TCP 2001:db8::1 80\n");
else
@@ -99,33 +105,36 @@ check_one (void)
static int
do_test (void)
{
- for (int do_single_lookup = 0; do_single_lookup < 2; ++do_single_lookup)
- {
- struct resolv_test *aux = resolv_test_start
- ((struct resolv_redirect_config)
- {
- .response_callback = response,
- });
+ for (do_strict_error = 0; do_strict_error < 2; ++do_strict_error)
+ for (int do_single_lookup = 0; do_single_lookup < 2; ++do_single_lookup)
+ {
+ struct resolv_test *aux = resolv_test_start
+ ((struct resolv_redirect_config)
+ {
+ .response_callback = response,
+ });
- if (do_single_lookup)
- _res.options |= RES_SNGLKUP;
+ if (do_strict_error)
+ _res.options |= RES_STRICTERR;
+ if (do_single_lookup)
+ _res.options |= RES_SNGLKUP;
- for (int do_fail_aaaa = 0; do_fail_aaaa < 2; ++do_fail_aaaa)
- {
- fail_aaaa = do_fail_aaaa;
+ for (int do_fail_aaaa = 0; do_fail_aaaa < 2; ++do_fail_aaaa)
+ {
+ fail_aaaa = do_fail_aaaa;
- rcode = 2; /* SERVFAIL. */
- check_one ();
+ rcode = 2; /* SERVFAIL. */
+ check_one ();
- rcode = 4; /* NOTIMP. */
- check_one ();
+ rcode = 4; /* NOTIMP. */
+ check_one ();
- rcode = 5; /* REFUSED. */
- check_one ();
- }
+ rcode = 5; /* REFUSED. */
+ check_one ();
+ }
- resolv_test_end (aux);
- }
+ resolv_test_end (aux);
+ }
return 0;
}
--
2.45.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890)
2024-06-14 6:20 ` [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890) Florian Weimer
@ 2024-06-14 7:43 ` Florian Weimer
2024-06-14 9:51 ` Sam James
2024-06-20 20:26 ` DJ Delorie
2 siblings, 0 replies; 14+ messages in thread
From: Florian Weimer @ 2024-06-14 7:43 UTC (permalink / raw)
To: libc-alpha
* 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890)
2024-06-14 6:20 ` [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890) Florian Weimer
2024-06-14 7:43 ` Florian Weimer
@ 2024-06-14 9:51 ` Sam James
2024-06-14 10:12 ` Florian Weimer
2024-06-20 20:26 ` DJ Delorie
2 siblings, 1 reply; 14+ messages in thread
From: Sam James @ 2024-06-14 9:51 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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"?
> [...]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890)
2024-06-14 9:51 ` Sam James
@ 2024-06-14 10:12 ` Florian Weimer
0 siblings, 0 replies; 14+ messages in thread
From: Florian Weimer @ 2024-06-14 10:12 UTC (permalink / raw)
To: Sam James; +Cc: libc-alpha
* 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] resolv: Do not wait for non-existing second DNS response after error (bug 30081)
2024-06-14 6:20 ` [PATCH 2/4] resolv: Do not wait for non-existing second DNS response after error (bug 30081) Florian Weimer
@ 2024-06-20 8:37 ` Andreas Schwab
2024-06-20 20:41 ` DJ Delorie
1 sibling, 0 replies; 14+ messages in thread
From: Andreas Schwab @ 2024-06-20 8:37 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
On Jun 14 2024, Florian Weimer wrote:
> In single-request mode, there is no second response after an error
> because the second query has not been sent yet. Waiting for it
> introduces an unnecessary timeout.
Ok.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890)
2024-06-14 6:20 ` [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890) Florian Weimer
2024-06-14 7:43 ` Florian Weimer
2024-06-14 9:51 ` Sam James
@ 2024-06-20 20:26 ` DJ Delorie
2024-07-01 13:27 ` Florian Weimer
2 siblings, 1 reply; 14+ messages in thread
From: DJ Delorie @ 2024-06-20 20:26 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] resolv: Do not wait for non-existing second DNS response after error (bug 30081)
2024-06-14 6:20 ` [PATCH 2/4] resolv: Do not wait for non-existing second DNS response after error (bug 30081) Florian Weimer
2024-06-20 8:37 ` Andreas Schwab
@ 2024-06-20 20:41 ` DJ Delorie
1 sibling, 0 replies; 14+ messages in thread
From: DJ Delorie @ 2024-06-20 20:41 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
Florian Weimer <fweimer@redhat.com> writes:
> + tst-resolv-semi-failure \
Ok.
> +$(objpfx)tst-resolv-semi-failure: $(objpfx)libresolv.so \
> + $(shared-thread-library)
Ok.
> diff --git a/resolv/res_send.c b/resolv/res_send.c
> index 572e72c32f..9c77613f37 100644
> --- a/resolv/res_send.c
> +++ b/resolv/res_send.c
> @@ -1238,7 +1238,7 @@ send_dg(res_state statp,
> *resplen2 = 0;
> return resplen;
> }
> - if (buf2 != NULL)
> + if (buf2 != NULL && !single_request)
Ok, don't check for the second answer if we didn't send a second query.
> diff --git a/resolv/tst-resolv-semi-failure.c b/resolv/tst-resolv-semi-failure.c
> new file mode 100644
> index 0000000000..aa9798b5a7
> --- /dev/null
> +++ b/resolv/tst-resolv-semi-failure.c
> @@ -0,0 +1,133 @@
> +/* Test parallel failure/success responses (bug 30081).
> + 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;
> +
> +/* Whether to fail the initial A query (!fail_aaaa) or the initial
> + AAAA query (fail_aaaa). */
> +static volatile bool fail_aaaa;
> +
> +static void
> +response (const struct resolv_response_context *ctx,
> + struct resolv_response_builder *b,
> + const char *qname, uint16_t qclass, uint16_t qtype)
> +{
> + /* Handle the failing query. */
> + if ((fail_aaaa && qtype == T_AAAA) && ctx->server_index == 0)
> + {
> + struct resolv_response_flags flags = {.rcode = rcode};
> + resolv_response_init (b, flags);
> + return;
> + }
Ok.
> + /* Otherwise produce a response. */
> + 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;
Ok.
> + default:
> + FAIL_EXIT1 ("unexpected TYPE%d query", qtype);
> + }
> + resolv_response_close_record (b);
> +}
Ok.
> +static void
> +check_one (void)
> +{
> +
> + /* The buggy 1-second query timeout results in 30 seconds of delay,
> + which triggers are test timeout failure. */
Same comment here - can we rely on this?
> + for (int i = 0; i < 30; ++i)
> + {
> + static const struct addrinfo hints =
> + {
> + .ai_family = AF_UNSPEC,
> + .ai_socktype = SOCK_STREAM,
> + };
> + struct addrinfo *ai;
> + int ret = getaddrinfo ("www.example", "80", &hints, &ai);
> + const char *expected;
> + if (ret == 0 && ai->ai_next != NULL)
> + expected = ("address: STREAM/TCP 192.0.2.17 80\n"
> + "address: STREAM/TCP 2001:db8::1 80\n");
> + else
> + /* Only one response because the AAAA lookup failure is
> + treated as an ignoreable error. */
> + expected = "address: STREAM/TCP 192.0.2.17 80\n";
> + check_addrinfo ("www.example", ai, ret, expected);
> + if (ret == 0)
> + freeaddrinfo (ai);
> + }
> +}
Ok.
> +static int
> +do_test (void)
> +{
> + for (int do_single_lookup = 0; do_single_lookup < 2; ++do_single_lookup)
> + {
> + struct resolv_test *aux = resolv_test_start
> + ((struct resolv_redirect_config)
> + {
> + .response_callback = response,
> + });
> +
> + if (do_single_lookup)
> + _res.options |= RES_SNGLKUP;
Ok.
> + for (int do_fail_aaaa = 0; do_fail_aaaa < 2; ++do_fail_aaaa)
> + {
> + fail_aaaa = do_fail_aaaa;
Ok.
> + rcode = 2; /* SERVFAIL. */
> + check_one ();
> +
> + rcode = 4; /* NOTIMP. */
> + check_one ();
> +
> + rcode = 5; /* REFUSED. */
> + check_one ();
> + }
> +
> + resolv_test_end (aux);
> + }
> +
> + return 0;
> +}
Ok.
> +#include <support/test-driver.c>
> diff --git a/resolv/tst-resolv-short-response.c b/resolv/tst-resolv-short-response.c
> index 0647de0704..a6ea456e46 100644
> --- a/resolv/tst-resolv-short-response.c
> +++ b/resolv/tst-resolv-short-response.c
> @@ -81,6 +81,18 @@ check_one (void)
> check_hostent ("www.example", gethostbyname2 ("www.example", AF_INET6),
> "name: www.example\n"
> "address: 2001:db8::1\n");
> + static const struct addrinfo hints =
> + {
> + .ai_family = AF_UNSPEC,
> + .ai_socktype = SOCK_STREAM,
> + };
> + struct addrinfo *ai;
> + int ret = getaddrinfo ("www.example", "80", &hints, &ai);
> + check_addrinfo ("www.example", ai, ret,
> + "address: STREAM/TCP 192.0.2.17 80\n"
> + "address: STREAM/TCP 2001:db8::1 80\n");
> + if (ret == 0)
> + freeaddrinfo (ai);
> }
> }
Ok.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] resolv: Support clearing option flags with a “-” prefix (bug 14799)
2024-06-14 6:20 ` [PATCH 3/4] resolv: Support clearing option flags with a “-” prefix (bug 14799) Florian Weimer
@ 2024-06-20 21:06 ` DJ Delorie
0 siblings, 0 replies; 14+ messages in thread
From: DJ Delorie @ 2024-06-20 21:06 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
Florian Weimer <fweimer@redhat.com> writes:
> I think using a “-” prefix is less confusing than introducing
> double-negation construct (“no-no-tld-query”).
> +* In /etc/resolv.conf and the RES_OPTIONS, option flags can now be
> + prefixed with “-” to clear previously set flags. For example, if
> + /etc/resolv.conf contains “options no-aaaa”, a process running with
> + the RES_OPTIONS=-no-aaaa environment variable performs AAAA DNS
> + queries when the glibc DNS stub resolver is used.
Ok.
> diff --git a/resolv/res_init.c b/resolv/res_init.c
> - uint8_t clear;
Ok, field removed.
> #define STRnLEN(str) str, sizeof (str) - 1
> - { STRnLEN ("rotate"), 0, RES_ROTATE },
> - { STRnLEN ("edns0"), 0, RES_USE_EDNS0 },
> - { STRnLEN ("single-request-reopen"), 0, RES_SNGLKUPREOP },
> - { STRnLEN ("single-request"), 0, RES_SNGLKUP },
> - { STRnLEN ("no_tld_query"), 0, RES_NOTLDQUERY },
> - { STRnLEN ("no-tld-query"), 0, RES_NOTLDQUERY },
> - { STRnLEN ("no-reload"), 0, RES_NORELOAD },
> - { STRnLEN ("use-vc"), 0, RES_USEVC },
> - { STRnLEN ("trust-ad"), 0, RES_TRUSTAD },
> - { STRnLEN ("no-aaaa"), 0, RES_NOAAAA },
> + { STRnLEN ("rotate"), RES_ROTATE },
> + { STRnLEN ("edns0"), RES_USE_EDNS0 },
> + { STRnLEN ("single-request-reopen"), RES_SNGLKUPREOP },
> + { STRnLEN ("single-request"), RES_SNGLKUP },
> + { STRnLEN ("no_tld_query"), RES_NOTLDQUERY },
> + { STRnLEN ("no-tld-query"), RES_NOTLDQUERY },
> + { STRnLEN ("no-reload"), RES_NORELOAD },
> + { STRnLEN ("use-vc"), RES_USEVC },
> + { STRnLEN ("trust-ad"), RES_TRUSTAD },
> + { STRnLEN ("no-aaaa"), RES_NOAAAA },
Ok. No change other than to remove the unneeded 0.
> #define noptions (sizeof (options) / sizeof (options[0]))
> + bool negate_option = *cp == '-';
> + if (negate_option)
> + ++cp;
Ok.
> for (int i = 0; i < noptions; ++i)
> if (strncmp (cp, options[i].str, options[i].len) == 0)
> {
> - if (options[i].clear)
> - parser->template.options &= options[i].flag;
> + if (negate_option)
> + parser->template.options &= ~options[i].flag;
> else
> parser->template.options |= options[i].flag;
Ok.
> diff --git a/resolv/tst-resolv-res_init-skeleton.c b/resolv/tst-resolv-res_init-skeleton.c
> index 6bef62cde2..d3a19eb305 100644
> --- a/resolv/tst-resolv-res_init-skeleton.c
> +++ b/resolv/tst-resolv-res_init-skeleton.c
> @@ -679,6 +679,16 @@ struct test_case test_cases[] =
> "; nameserver[0]: [192.0.2.1]:53\n",
> .res_options = "attempts:5 ndots:3 edns0 ",
> },
> + {.name = "RES_OPTIONS can clear flags",
> + .conf = "options ndots:2 use-vc no-aaaa edns0\n"
> + "nameserver 192.0.2.1\n",
> + .expected = "options ndots:3 use-vc\n"
> + "search example.com\n"
> + "; search[0]: example.com\n"
Where does this search come from? Unrelated to what you're testing, but...
(ok as long as the test succeeds, I suppose :)
> + "nameserver 192.0.2.1\n"
> + "; nameserver[0]: [192.0.2.1]:53\n",
> + .res_options = "ndots:3 -edns0 -no-aaaa",
> + },
Otherwise OK.
LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] resolv: Implement strict-error stub resolver option (bug 27929)
2024-06-14 6:21 ` [PATCH 4/4] resolv: Implement strict-error stub resolver option (bug 27929) Florian Weimer
@ 2024-06-20 21:24 ` DJ Delorie
0 siblings, 0 replies; 14+ messages in thread
From: DJ Delorie @ 2024-06-20 21:24 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
Florian Weimer <fweimer@redhat.com> writes:
> For now, do not enable this mode by default due to the potential
> impact on compatibility with existing deployments.
> +* The DNS stub resolver now supports the strict-error option. If
> + activated, getaddrinfo for the AF_UNSPEC address family (with dual
> + A/AAAA DNS lookups) attemps to obtain a A/AAAA response pair from
s/a/an/
> + another DNS server if one of the responses indicates failure. Without
> + the strict-error option, getaddrinfo returns the A record data it has
> + obtained even if the AAAA query failed. The new strict error mode is
> + incompatible with some DNS environments which do not follow the RFCs,
> + which is why this mode is not enabled by default. A future version
> + of the library may turn it on by default, however.
Ok.
> diff --git a/resolv/res_init.c b/resolv/res_init.c
> + { STRnLEN ("strict-error"), RES_STRICTERR },
Ok.
> diff --git a/resolv/res_send.c b/resolv/res_send.c
> if (thisansp_error) {
> next_ns:
> - if (recvresp1 || (buf2 != NULL && recvresp2)) {
> - *resplen2 = 0;
> - return resplen;
> - }
Moved later, ok.
> - if (buf2 != NULL && !single_request)
Moved later, ok.
> + /* Outside of strict-error mode, use the first
> + response even if the second response is an
> + error. This allows parallel resolution to
> + succeed even if the recursive resolver
> + always answers with SERVFAIL for AAAA
> + queries (which still happens in practice
> + unfortunately).
> +
> + In strict-error mode, always switch to the
> + next server and try to get a response from
> + there. */
> + if ((statp->options & RES_STRICTERR) == 0)
Ok.
> {
> - /* No data from the first reply. */
> - resplen = 0;
> - /* We are waiting for a possible second reply. */
> - if (matching_query == 1)
> - recvresp1 = 1;
> - else
> - recvresp2 = 1;
> -
> - goto wait;
Moved later, ok.
> + if (recvresp1 || (buf2 != NULL && recvresp2))
> + {
> + *resplen2 = 0;
> + return resplen;
> + }
> +
> + if (buf2 != NULL && !single_request)
> + {
> + /* No data from the first reply. */
> + resplen = 0;
> + /* We are waiting for a possible
> + second reply. */
> + if (matching_query == 1)
> + recvresp1 = 1;
> + else
> + recvresp2 = 1;
> +
> + goto wait;
> + }
> }
Ok.
> diff --git a/resolv/resolv.h b/resolv/resolv.h
> index f40d6c58ce..b8a0f66a5f 100644
> --- a/resolv/resolv.h
> +++ b/resolv/resolv.h
> @@ -133,6 +133,7 @@ struct res_sym {
> #define RES_NORELOAD 0x02000000 /* No automatic configuration reload. */
> #define RES_TRUSTAD 0x04000000 /* Request AD bit, keep it in responses. */
> #define RES_NOAAAA 0x08000000 /* Suppress AAAA queries. */
> +#define RES_STRICTERR 0x10000000 /* Report more DNS errors as errors. */
Ok.
> print_option_flag (fp, &options, RES_NOAAAA, "no-aaaa");
> + print_option_flag (fp, &options, RES_STRICTERR, "strict-error");
Ok.
> @@ -741,6 +742,15 @@ struct test_case test_cases[] =
> "nameserver 192.0.2.1\n"
> "; nameserver[0]: [192.0.2.1]:53\n"
> },
> + {.name = "strict-error flag",
> + .conf = "options strict-error\n"
> + "nameserver 192.0.2.1\n",
> + .expected = "options strict-error\n"
> + "search example.com\n"
> + "; search[0]: example.com\n"
> + "nameserver 192.0.2.1\n"
> + "; nameserver[0]: [192.0.2.1]:53\n"
> + },
Ok.
> diff --git a/resolv/tst-resolv-semi-failure.c b/resolv/tst-resolv-semi-failure.c
>
> +/* Set to 1 if strict error checking is enabled. */
> +static int do_strict_error;
Ok.
> static void
> check_one (void)
> {
> @@ -83,7 +86,10 @@ check_one (void)
> struct addrinfo *ai;
> int ret = getaddrinfo ("www.example", "80", &hints, &ai);
> const char *expected;
> - if (ret == 0 && ai->ai_next != NULL)
> + /* In strict-error mode, a switch to the second name server
> + happens, and both responses are received, so a single
> + response is a bug. */
> + if (do_strict_error || (ret == 0 && ai->ai_next != NULL))
> expected = ("address: STREAM/TCP 192.0.2.17 80\n"
> "address: STREAM/TCP 2001:db8::1 80\n");
Ok.
> @@ -99,33 +105,36 @@ check_one (void)
> static int
> do_test (void)
> {
> + for (do_strict_error = 0; do_strict_error < 2; ++do_strict_error)
Ok.
Everything else is mostly just indentation...
> + if (do_strict_error)
> + _res.options |= RES_STRICTERR;
Ok.
LGTM aside from that one tiny typo.
Reviewed-by: DJ Delorie <dj@redhat.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890)
2024-06-20 20:26 ` DJ Delorie
@ 2024-07-01 13:27 ` Florian Weimer
0 siblings, 0 replies; 14+ messages in thread
From: Florian Weimer @ 2024-07-01 13:27 UTC (permalink / raw)
To: DJ Delorie; +Cc: libc-alpha
* 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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-07-01 13:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-06-14 6:20 [PATCH 0/4] Some DNS stub resolver fixes/enhancements Florian Weimer
2024-06-14 6:20 ` [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890) Florian Weimer
2024-06-14 7:43 ` Florian Weimer
2024-06-14 9:51 ` Sam James
2024-06-14 10:12 ` Florian Weimer
2024-06-20 20:26 ` DJ Delorie
2024-07-01 13:27 ` Florian Weimer
2024-06-14 6:20 ` [PATCH 2/4] resolv: Do not wait for non-existing second DNS response after error (bug 30081) Florian Weimer
2024-06-20 8:37 ` Andreas Schwab
2024-06-20 20:41 ` DJ Delorie
2024-06-14 6:20 ` [PATCH 3/4] resolv: Support clearing option flags with a “-” prefix (bug 14799) Florian Weimer
2024-06-20 21:06 ` DJ Delorie
2024-06-14 6:21 ` [PATCH 4/4] resolv: Implement strict-error stub resolver option (bug 27929) Florian Weimer
2024-06-20 21:24 ` DJ Delorie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).