[PATCH 0/4] Some DNS stub resolver fixes/enhancements

[PATCH 0/4] Some DNS stub resolver fixes/enhancements

[Florian Weimer]

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

[PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890)

[Florian Weimer]

---
 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

Re: [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890)

[Florian Weimer]

* 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

Re: [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890)

[Sam James]

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"?

> [...]

Re: [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890)

[Florian Weimer]

* 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

Re: [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890)

[DJ Delorie]

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.

Re: [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890)

[Florian Weimer]

* 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

Re: [PATCH 1/4] resolv: Allow short error responses to match any query (bug 31890)

[DJ Delorie]

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>

[PATCH 2/4] resolv: Do not wait for non-existing second DNS response after error (bug 30081)

[Florian Weimer]

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

Re: [PATCH 2/4] resolv: Do not wait for non-existing second DNS response after error (bug 30081)

[Andreas Schwab]

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."

Re: [PATCH 2/4] resolv: Do not wait for non-existing second DNS response after error (bug 30081)

[DJ Delorie]

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.

Re: [PATCH 2/4] resolv: Do not wait for non-existing second DNS response after error (bug 30081)

[Florian Weimer]

* 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.  */
>
> Same comment here - can we rely on this?

Same argument here—I think it's okay if this test fails only with the
default TIMEOUTFACTOR.

Thanks,
Florian

Re: [PATCH 2/4] resolv: Do not wait for non-existing second DNS response after error (bug 30081)

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
>>> +  /* 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?
>
> Same argument here—I think it's okay if this test fails only with the
> default TIMEOUTFACTOR.

Same response here ;-)

Reviewed-by: DJ Delorie <dj@redhat.com>

[PATCH 3/4] resolv: Support clearing option flags with a “-” prefix (bug 14799)

[Florian Weimer]

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

Re: [PATCH 3/4] resolv: Support clearing option flags with a “-” prefix (bug 14799)

[DJ Delorie]

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>

[PATCH 4/4] resolv: Implement strict-error stub resolver option (bug 27929)

[Florian Weimer]

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

Re: [PATCH 4/4] resolv: Implement strict-error stub resolver option (bug 27929)

[DJ Delorie]

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>