[COMMITTED] nss_dns: Remove dead PTR IPv4-to-IPv6 mapping code

Message ID 20170906131329.924C94194EE17@oldenburg.str.redhat.com
State New
Headers show
Series
  • [COMMITTED] nss_dns: Remove dead PTR IPv4-to-IPv6 mapping code
Related show

Commit Message

Florian Weimer Sept. 6, 2017, 1:13 p.m.
2017-09-06  Florian Weimer  <fweimer@redhat.com>

	Remove dead PTR IPv4-to-IPv6 mapping code from nss_dns.
	* resolv/nss_dns/dns-host.c (getanswer_r): Remove dead code.
	* resolv/tst-res_use_inet6.c (response_ptr_v4, response_ptr_v6):
	New functions.
	(response): Call them.  Add 'p', '6' flag processing.
	(test_reverse): New function.
	(test_get2_any): Call it.
	(test_no_inet6): Add 'p' test.
	(test_inet6): Likewise.

Comments

Andreas Schwab Sept. 6, 2017, 1:32 p.m. | #1
On Sep 06 2017, fweimer@redhat.com (Florian Weimer) wrote:

> 	Remove dead PTR IPv4-to-IPv6 mapping code from nss_dns.
> 	* resolv/nss_dns/dns-host.c (getanswer_r): Remove dead code.

In which way is that dead?

Andreas.
Florian Weimer Sept. 6, 2017, 1:43 p.m. | #2
On 09/06/2017 03:32 PM, Andreas Schwab wrote:
> On Sep 06 2017, fweimer@redhat.com (Florian Weimer) wrote:
> 
>> 	Remove dead PTR IPv4-to-IPv6 mapping code from nss_dns.
>> 	* resolv/nss_dns/dns-host.c (getanswer_r): Remove dead code.
> 
> In which way is that dead?

      if (type == T_A && qtype == T_AAAA && map)
	have_to_map = 1;
      else if (__glibc_unlikely (type != qtype))
	{
	  cp += n;
	  continue;			/* XXX - had_error++ ? */
	}

So have_to_map being true implies that qtype == T_AAAA.  But this means
that after the if statement, type == T_A || type == T_AAAA.  In
particular, type != T_PTR, which is why the have_to_map check is
superfluous.

The added tests also show it's dead code.

Thanks,
Florian
Andreas Schwab Sept. 6, 2017, 1:51 p.m. | #3
On Sep 06 2017, Florian Weimer <fweimer@redhat.com> wrote:

> On 09/06/2017 03:32 PM, Andreas Schwab wrote:
>> On Sep 06 2017, fweimer@redhat.com (Florian Weimer) wrote:
>> 
>>> 	Remove dead PTR IPv4-to-IPv6 mapping code from nss_dns.
>>> 	* resolv/nss_dns/dns-host.c (getanswer_r): Remove dead code.
>> 
>> In which way is that dead?
>
>       if (type == T_A && qtype == T_AAAA && map)
> 	have_to_map = 1;
>       else if (__glibc_unlikely (type != qtype))
> 	{
> 	  cp += n;
> 	  continue;			/* XXX - had_error++ ? */
> 	}
>
> So have_to_map being true implies that qtype == T_AAAA.  But this means
> that after the if statement, type == T_A || type == T_AAAA.  In
> particular, type != T_PTR, which is why the have_to_map check is
> superfluous.

But it's inside a loop, and have_to_map is not reset.

Andreas.
Florian Weimer Sept. 6, 2017, 1:56 p.m. | #4
On 09/06/2017 03:51 PM, Andreas Schwab wrote:
> On Sep 06 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> On 09/06/2017 03:32 PM, Andreas Schwab wrote:
>>> On Sep 06 2017, fweimer@redhat.com (Florian Weimer) wrote:
>>>
>>>> 	Remove dead PTR IPv4-to-IPv6 mapping code from nss_dns.
>>>> 	* resolv/nss_dns/dns-host.c (getanswer_r): Remove dead code.
>>>
>>> In which way is that dead?
>>
>>       if (type == T_A && qtype == T_AAAA && map)
>> 	have_to_map = 1;
>>       else if (__glibc_unlikely (type != qtype))
>> 	{
>> 	  cp += n;
>> 	  continue;			/* XXX - had_error++ ? */
>> 	}
>>
>> So have_to_map being true implies that qtype == T_AAAA.  But this means
>> that after the if statement, type == T_A || type == T_AAAA.  In
>> particular, type != T_PTR, which is why the have_to_map check is
>> superfluous.
> 
> But it's inside a loop, and have_to_map is not reset.

And neither is qtype, which is why the type == T_A || type == T_AAAA
post-condition still holds.

Florian

Patch

diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
index 7cd54ab504..1e85e4f08f 100644
--- a/resolv/nss_dns/dns-host.c
+++ b/resolv/nss_dns/dns-host.c
@@ -889,19 +889,6 @@  getanswer_r (struct resolv_context *ctx,
 	  /* bind would put multiple PTR records as aliases, but we don't do
 	     that.  */
 	  result->h_name = bp;
-	  if (have_to_map)
-	    {
-	      n = strlen (bp) + 1;	/* for the \0 */
-	      if (__glibc_unlikely (n >= MAXHOSTNAMELEN))
-		{
-		  ++had_error;
-		  break;
-		}
-	      bp += n;
-	      linebuflen -= n;
-	      if (map_v4v6_hostent (result, &bp, &linebuflen))
-		goto too_small;
-	    }
 	  *h_errnop = NETDB_SUCCESS;
 	  return NSS_STATUS_SUCCESS;
 	case T_A:
diff --git a/resolv/tst-res_use_inet6.c b/resolv/tst-res_use_inet6.c
index 1522d5c5f5..d819f921d6 100644
--- a/resolv/tst-res_use_inet6.c
+++ b/resolv/tst-res_use_inet6.c
@@ -16,31 +16,101 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#include <ctype.h>
 #include <netdb.h>
 #include <resolv.h>
+#include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include <support/check.h>
 #include <support/check_nss.h>
 #include <support/resolv_test.h>
+#include <support/support.h>
 #include <support/xthread.h>
 
+/* Handle IPv4 reverse lookup responses.  Product a PTR record
+   A-B-C-D.v4.example.  */
+static void
+response_ptr_v4 (const struct resolv_response_context *ctx,
+                 struct resolv_response_builder *b,
+                 const char *qname, uint16_t qclass, uint16_t qtype)
+{
+  int bytes[4];
+  int offset = -1;
+  TEST_VERIFY (sscanf (qname, "%d.%d.%d.%d.in-addr.arpa%n",
+                       bytes + 0, bytes + 1, bytes + 2, bytes + 3,
+                       &offset) == 4);
+  TEST_VERIFY (offset == strlen (qname));
+  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, T_PTR, 0);
+  char *name = xasprintf ("%d-%d-%d-%d.v4.example",
+                          bytes[3], bytes[2], bytes[1], bytes[0]);
+  resolv_response_add_name (b, name);
+  free (name);
+  resolv_response_close_record (b);
+}
+
+/* Handle IPv6 reverse lookup responses.  Produce a PTR record
+   <32 hex digits>.v6.example. */
+static void
+response_ptr_v6 (const struct resolv_response_context *ctx,
+                 struct resolv_response_builder *b,
+                 const char *qname, uint16_t qclass, uint16_t qtype)
+{
+
+  TEST_VERIFY_EXIT (strlen (qname) > 64);
+
+  char bytes[33];
+  for (int i = 0; i < 64; ++i)
+    if ((i % 2) == 0)
+      {
+        TEST_VERIFY (isxdigit ((unsigned char) qname[i]));
+        bytes[31 - i / 2] = qname[i];
+      }
+    else
+      TEST_VERIFY_EXIT (qname[i] == '.');
+  bytes[32] = '\0';
+
+    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, T_PTR, 0);
+  char *name = xasprintf ("%s.v6.example", bytes);
+  resolv_response_add_name (b, name);
+  free (name);
+  resolv_response_close_record (b);
+}
+
 /* Produce a response based on QNAME: Certain characters in the first
    label of QNAME trigger the inclusion of resource records:
 
    'a'   A record (IPv4 address)
    'q'   AAAA record (quad A record, IPv6 address)
+   'p'   PTR record
    'm'   record type must match QTYPE (no additional records)
+   '6'   stop flag processing if QTYPE == AAAA
 
-   QTYPE is ignored for record type selection if 'm' is not
-   specified.  */
+   For 'a' and 'q', QTYPE is ignored for record type selection if 'm'
+   is not specified.
+
+   in-addr.arpa and ip6.arpa queries are handled separately in
+   response_ptr_v4 and response_ptr_v6.  */
 static void
 response (const struct resolv_response_context *ctx,
           struct resolv_response_builder *b,
           const char *qname, uint16_t qclass, uint16_t qtype)
 {
+  if (strstr (qname, ".in-addr.arpa") != NULL)
+    return response_ptr_v4 (ctx, b, qname, qclass, qtype);
+  else if (strstr (qname, ".ip6.arpa") != NULL)
+    return response_ptr_v6 (ctx, b, qname, qclass, qtype);
+
   bool include_a = false;
   bool include_aaaa = false;
   bool include_match = false;
+  bool include_ptr = false;
   for (const char *p = qname; *p != '.' && *p != '\0'; ++p)
     {
       if (*p == 'a')
@@ -49,6 +119,10 @@  response (const struct resolv_response_context *ctx,
         include_aaaa = true;
       else if (*p == 'm')
         include_match = true;
+      else if (*p == 'p')
+        include_ptr = true;
+      else if (*p == '6' && qtype == T_AAAA)
+        break;
     }
   if (include_match)
     {
@@ -70,11 +144,17 @@  response (const struct resolv_response_context *ctx,
     }
   if (include_aaaa)
     {
-        char ipv6[16]
-          = {0x20, 0x01, 0xd, 0xb8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1};
-        resolv_response_open_record (b, qname, qclass, T_AAAA, 0);
-        resolv_response_add_data (b, &ipv6, sizeof (ipv6));
-        resolv_response_close_record (b);
+      char ipv6[16]
+        = {0x20, 0x01, 0xd, 0xb8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1};
+      resolv_response_open_record (b, qname, qclass, T_AAAA, 0);
+      resolv_response_add_data (b, &ipv6, sizeof (ipv6));
+      resolv_response_close_record (b);
+    }
+  if (include_ptr)
+    {
+      resolv_response_open_record (b, qname, qclass, T_PTR, 0);
+      resolv_response_add_name (b, "ptr-target.example");
+      resolv_response_close_record (b);
     }
 }
 
@@ -162,6 +242,65 @@  test_gai (void)
   }
 }
 
+/* Test gethostbyaddr and getnameinfo.  The results are independent of
+   RES_USE_INET6.  */
+static void
+test_reverse (void)
+{
+  {
+    char ipv4[4] = { 192, 0, 2, 17 };
+    check_hostent ("gethostbyaddr AF_INET",
+                   gethostbyaddr (ipv4, sizeof (ipv4), AF_INET),
+                   "name: 192-0-2-17.v4.example\n"
+                   "address: 192.0.2.17\n");
+  }
+  {
+    char ipv6[16]
+      = {0x20, 0x01, 0xd, 0xb8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1};
+    check_hostent ("gethostbyaddr AF_INET",
+                   gethostbyaddr (ipv6, sizeof (ipv6), AF_INET6),
+                   "name: 20010db8000000000000000000000001.v6.example\n"
+                   "address: 2001:db8::1\n");
+  }
+
+  {
+    struct sockaddr_in addr =
+      {
+        .sin_family = AF_INET,
+        .sin_addr = { .s_addr = htonl (0xc0000211) },
+        .sin_port = htons (80)
+      };
+    char host[NI_MAXHOST];
+    char service[NI_MAXSERV];
+    int ret = getnameinfo ((struct sockaddr *) &addr, sizeof (addr),
+                           host, sizeof (host), service, sizeof (service),
+                           NI_NUMERICSERV);
+    TEST_VERIFY (ret == 0);
+    TEST_VERIFY (strcmp (host, "192-0-2-17.v4.example") == 0);
+    TEST_VERIFY (strcmp (service, "80") == 0);
+  }
+  {
+    char ipv6[16]
+      = {0x20, 0x01, 0xd, 0xb8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1};
+    struct sockaddr_in6 addr =
+      {
+        .sin6_family = AF_INET6,
+        .sin6_port = htons (80),
+      };
+    TEST_VERIFY (sizeof (ipv6) == sizeof (addr.sin6_addr));
+    memcpy (&addr.sin6_addr, ipv6, sizeof (addr.sin6_addr));
+    char host[NI_MAXHOST];
+    char service[NI_MAXSERV];
+    int ret = getnameinfo ((struct sockaddr *) &addr, sizeof (addr),
+                           host, sizeof (host), service, sizeof (service),
+                           NI_NUMERICSERV);
+    TEST_VERIFY (ret == 0);
+    TEST_VERIFY
+      (strcmp (host, "20010db8000000000000000000000001.v6.example") == 0);
+    TEST_VERIFY (strcmp (service, "80") == 0);
+  }
+}
+
 /* Test that gethostbyname2 is mostly not influenced by
    RES_USE_INET6.  */
 static void
@@ -207,6 +346,8 @@  test_get2_any (void)
                  "name: qa.example\n"
                  "address: 2001:db8::1\n");
   /* Additional AF_INET6 tests depend on RES_USE_INET6; see below.  */
+
+  test_reverse ();
 }
 
 /* gethostbyname2 tests with RES_USE_INET6 disabled.  */
@@ -254,6 +395,10 @@  test_no_inet6 (void)
                  gethostbyname ("am.example"),
                  "name: am.example\n"
                  "address: 192.0.2.17\n");
+  check_hostent ("gethostbyname (\"amp.example\")",
+                 gethostbyname ("amp.example"),
+                 "name: amp.example\n"
+                 "address: 192.0.2.17\n");
   check_hostent ("gethostbyname (\"qam.example\")",
                  gethostbyname ("qam.example"),
                  "name: qam.example\n"
@@ -307,6 +452,28 @@  threadfunc (void *ignored)
                  gethostbyname ("qm.inet6.example"),
                  "name: qm.inet6.example\n"
                  "address: 2001:db8::1\n");
+  check_hostent ("gethostbyname (\"amp.inet6.example\")",
+                 gethostbyname ("amp.inet6.example"),
+                 "error: NO_RECOVERY\n");
+  check_hostent ("gethostbyname (\"qmp.inet6.example\")",
+                 gethostbyname ("qmp.inet6.example"),
+                 "name: qmp.inet6.example\n"
+                 "address: 2001:db8::1\n");
+  check_hostent ("gethostbyname (\"ap.inet6.example\")",
+                 gethostbyname ("ap.inet6.example"),
+                 "error: NO_RECOVERY\n");
+  check_hostent ("gethostbyname (\"6ap.inet6.example\")",
+                 gethostbyname ("6ap.inet6.example"),
+                 "name: 6ap.inet6.example\n"
+                 "address: ::ffff:192.0.2.17\n");
+  check_hostent ("gethostbyname (\"am6p.inet6.example\")",
+                 gethostbyname ("am6p.inet6.example"),
+                 "name: am6p.inet6.example\n"
+                 "address: ::ffff:192.0.2.17\n");
+  check_hostent ("gethostbyname (\"qp.inet6.example\")",
+                 gethostbyname ("qp.inet6.example"),
+                 "name: qp.inet6.example\n"
+                 "address: 2001:db8::1\n");
   test_get2_inet6 ();
   test_get2_inet6 ();
   test_gai ();