diff mbox

resolv: Do not copy IPv4 addresses to IPv6 address array

Message ID 8113b7c9-b9a8-7157-f957-53853dc4e232@redhat.com
State New
Headers show

Commit Message

Florian Weimer June 29, 2017, 6:35 p.m. UTC 
While working on the resolver code, I discovered some address copying
which appears to be unnecessary.  The attached patch removes that.

The comments refer to timing data, but we currently do not have that.

Andreas, if I recall correctly, you wrote get_nsaddr.  What do you think
about this change?

Thanks,
Florian

Comments

Andreas Schwab July 3, 2017, 8:40 p.m. UTC | #1 
On Jun 29 2017, Florian Weimer <fweimer@redhat.com> wrote:

> Andreas, if I recall correctly, you wrote get_nsaddr.

Yes, the BIND resolver in FreeBSD has the same function.  Their way to
extend _res for IPv6 is worth looking at.

Andreas.
Florian Weimer July 4, 2017, 9:47 a.m. UTC | #2 
On 07/03/2017 10:40 PM, Andreas Schwab wrote:
> On Jun 29 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> Andreas, if I recall correctly, you wrote get_nsaddr.
> 
> Yes, the BIND resolver in FreeBSD has the same function.

What do you think about the patch I posted?

> Their way to extend _res for IPv6 is worth looking at.

They put pointers to malloc'ed state directly into _res.  We could od as
well, but it will cause problems if applications copy _res, so I went
for the more complex approach.

Thanks,
Florian
Carlos O'Donell July 4, 2017, 12:55 p.m. UTC | #3 
On 07/04/2017 05:47 AM, Florian Weimer wrote:
> On 07/03/2017 10:40 PM, Andreas Schwab wrote:
>> On Jun 29 2017, Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> Andreas, if I recall correctly, you wrote get_nsaddr.
>>
>> Yes, the BIND resolver in FreeBSD has the same function.
> 
> What do you think about the patch I posted?
> 
>> Their way to extend _res for IPv6 is worth looking at.
> 
> They put pointers to malloc'ed state directly into _res.  We could od as
> well, but it will cause problems if applications copy _res, so I went
> for the more complex approach.

A design that puts malloc'd pointers directly into _res is not a good idea
for the reasons you note. I agree with your solution.

Several kinds of "handle" interfaces have internal mapping constructs that
let you go from a "handle" (index XOR magic in this case) to a much more
complicated structure that can have reference counts and other data. This
is much more flexible than just a pointer to the data. In some cases it is
more costly than the simple interface, but in this case I think it's worth
the added complexity given what you're trying to achieve.
Andreas Schwab July 4, 2017, 4:53 p.m. UTC | #4 
On Jul 04 2017, Carlos O'Donell <carlos@redhat.com> wrote:

> A design that puts malloc'd pointers directly into _res is not a good idea
> for the reasons you note. I agree with your solution.

That's what we do.

Andreas.
Carlos O'Donell July 4, 2017, 6:05 p.m. UTC | #5 
On 07/04/2017 12:53 PM, Andreas Schwab wrote:
> On Jul 04 2017, Carlos O'Donell <carlos@redhat.com> wrote:
> 
>> A design that puts malloc'd pointers directly into _res is not a good idea
>> for the reasons you note. I agree with your solution.
> 
> That's what we do.

And it's not a good idea.
Florian Weimer July 27, 2017, 9:49 a.m. UTC | #6 
On 07/04/2017 08:05 PM, Carlos O'Donell wrote:
> On 07/04/2017 12:53 PM, Andreas Schwab wrote:
>> On Jul 04 2017, Carlos O'Donell <carlos@redhat.com> wrote:
>>
>>> A design that puts malloc'd pointers directly into _res is not a good idea
>>> for the reasons you note. I agree with your solution.
>>
>> That's what we do.
> 
> And it's not a good idea.

I didn't realize that res_send.c also allocated name server addresses
for IPv4 servers.  I assumed there weren't any allocations at all for
IPv4 addresses because __res_vinit does not perform such allocations.

These allocations are really old.  It suggests that we perhaps could
have used a more direct mechanism for storing the pointer to the
extended configuration.

Thanks,
Florian
diff mbox

Patch

resolv: Do not copy IPv4 addresses to IPv6 address array

get_nsaddr already prefers IPv4 addresses if they are present
(af_family != 0).  If we ensure that nssocks is initialized to -1
and sockets are closed even without an allocated server address,
there is no need to make these copies anymore.

2017-06-29  Florian Weimer  <fweimer@redhat.com>

	* resolv/bits/types/res_state.h (struct __res_state): Rename
	_u._ext.nscount to _u._ext.__glibc_unused_nscount.
	* resolv/res_init.c (res_vinit): Adjust.  Initialize nssocks
	member.
	* resolv/res_send.c (__libc_res_nsend): Do not copy IPv4
	nameserver addresses.
	* resolv/res-close.c (__res_iclose): Close sockets even if the
	name server address has not been allocated.
	* support/resolv_test.c (resolv_test_start): Do not clear
	_res._u._ext.nscount because it is unused.

diff --git a/resolv/bits/types/res_state.h b/resolv/bits/types/res_state.h
index cee4b6d..d274cf3 100644
--- a/resolv/bits/types/res_state.h
+++ b/resolv/bits/types/res_state.h
@@ -40,7 +40,7 @@  struct __res_state {
 	union {
 		char	pad[52];	/* On an i386 this means 512b total. */
 		struct {
-			uint16_t		nscount;
+			uint16_t		__glibc_unused_nscount;
 			uint16_t		nsmap[MAXNS];
 			int			nssocks[MAXNS];
 			uint16_t		nscount6;
diff --git a/resolv/res-close.c b/resolv/res-close.c
index 73f18d1..3195298 100644
--- a/resolv/res-close.c
+++ b/resolv/res-close.c
@@ -97,19 +97,18 @@  __res_iclose (res_state statp, bool free_addr)
       statp->_flags &= ~(RES_F_VC | RES_F_CONN);
     }
   for (int ns = 0; ns < statp->nscount; ns++)
-    if (statp->_u._ext.nsaddrs[ns] != NULL)
-      {
-        if (statp->_u._ext.nssocks[ns] != -1)
-          {
-            close_not_cancel_no_status (statp->_u._ext.nssocks[ns]);
-            statp->_u._ext.nssocks[ns] = -1;
-          }
-        if (free_addr)
-          {
-            free (statp->_u._ext.nsaddrs[ns]);
-            statp->_u._ext.nsaddrs[ns] = NULL;
-          }
-      }
+    {
+      if (statp->_u._ext.nssocks[ns] != -1)
+        {
+          close_not_cancel_no_status (statp->_u._ext.nssocks[ns]);
+          statp->_u._ext.nssocks[ns] = -1;
+        }
+      if (free_addr)
+        {
+          free (statp->_u._ext.nsaddrs[ns]);
+          statp->_u._ext.nsaddrs[ns] = NULL;
+        }
+    }
 }
 libc_hidden_def (__res_iclose)
 
diff --git a/resolv/res_init.c b/resolv/res_init.c
index 821f060..cd42ac6 100644
--- a/resolv/res_init.c
+++ b/resolv/res_init.c
@@ -155,9 +155,12 @@  res_vinit_1 (res_state statp, bool preinit, FILE *fp, char **buffer)
   statp->_flags = 0;
   statp->__glibc_unused_qhook = NULL;
   statp->__glibc_unused_rhook = NULL;
-  statp->_u._ext.nscount = 0;
+  statp->_u._ext.__glibc_unused_nscount = 0;
   for (int n = 0; n < MAXNS; n++)
-    statp->_u._ext.nsaddrs[n] = NULL;
+    {
+      statp->_u._ext.nsaddrs[n] = NULL;
+      statp->_u._ext.nssocks[n] = -1;
+    }
 
   /* Allow user to override the local domain definition.  */
   if ((cp = getenv ("LOCALDOMAIN")) != NULL)
@@ -328,7 +331,6 @@  res_vinit_1 (res_state statp, bool preinit, FILE *fp, char **buffer)
 
                       statp->nsaddr_list[nserv].sin_family = 0;
                       statp->_u._ext.nsaddrs[nserv] = sa6;
-                      statp->_u._ext.nssocks[nserv] = -1;
                       have_serv6 = true;
                       nserv++;
                     }
diff --git a/resolv/res_send.c b/resolv/res_send.c
index b7b8ecd..ef91d0f 100644
--- a/resolv/res_send.c
+++ b/resolv/res_send.c
@@ -373,54 +373,6 @@  __libc_res_nsend(res_state statp, const u_char *buf, int buflen,
 	terrno = ETIMEDOUT;
 
 	/*
-	 * If the ns_addr_list in the resolver context has changed, then
-	 * invalidate our cached copy and the associated timing data.
-	 */
-	if (EXT(statp).nscount != 0) {
-		int needclose = 0;
-
-		if (EXT(statp).nscount != statp->nscount)
-			needclose++;
-		else
-			for (ns = 0; ns < statp->nscount; ns++) {
-				if (statp->nsaddr_list[ns].sin_family != 0
-				    && !sock_eq((struct sockaddr_in6 *)
-						&statp->nsaddr_list[ns],
-						EXT(statp).nsaddrs[ns]))
-				{
-					needclose++;
-					break;
-				}
-			}
-		if (needclose) {
-			__res_iclose(statp, false);
-			EXT(statp).nscount = 0;
-		}
-	}
-
-	/*
-	 * Maybe initialize our private copy of the ns_addr_list.
-	 */
-	if (EXT(statp).nscount == 0) {
-		for (ns = 0; ns < statp->nscount; ns++) {
-			EXT(statp).nssocks[ns] = -1;
-			if (statp->nsaddr_list[ns].sin_family == 0)
-				continue;
-			if (EXT(statp).nsaddrs[ns] == NULL)
-				EXT(statp).nsaddrs[ns] =
-				    malloc(sizeof (struct sockaddr_in6));
-			if (EXT(statp).nsaddrs[ns] != NULL)
-				memset (mempcpy(EXT(statp).nsaddrs[ns],
-						&statp->nsaddr_list[ns],
-						sizeof (struct sockaddr_in)),
-					'\0',
-					sizeof (struct sockaddr_in6)
-					- sizeof (struct sockaddr_in));
-		}
-		EXT(statp).nscount = statp->nscount;
-	}
-
-	/*
 	 * Some resolvers want to even out the load on their nameservers.
 	 * Note that RES_BLAST overrides RES_ROTATE.
 	 */
diff --git a/support/resolv_test.c b/support/resolv_test.c
index 050cd71..b5df5ff 100644
--- a/support/resolv_test.c
+++ b/support/resolv_test.c
@@ -1103,7 +1103,6 @@  resolv_test_start (struct resolv_redirect_config config)
   /* Disable IPv6 name server addresses.  The code below only
      overrides the IPv4 addresses.  */
   __res_iclose (&_res, true);
-  _res._u._ext.nscount = 0;
 
   /* Redirect queries to the server socket.  */
   if (test_verbose)