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

Message ID 09ce1e84db1b175195584dd20aa7241f3e95061f.1718345824.git.fweimer@redhat.com
State Accepted
Headers
Series Some DNS stub resolver fixes/enhancements |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed

Commit Message

Florian Weimer June 14, 2024, 6:20 a.m. UTC
  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
  

Comments

Andreas Schwab June 20, 2024, 8:37 a.m. UTC | #1
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.
  
DJ Delorie June 20, 2024, 8:41 p.m. UTC | #2
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.
  
Florian Weimer July 2, 2024, 7:37 a.m. UTC | #3
* 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
  
DJ Delorie July 2, 2024, 4:48 p.m. UTC | #4
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

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);
     }
 }