* [PATCH] resolv: Do not copy IPv4 addresses to IPv6 address array
@ 2017-06-29 18:35 Florian Weimer
2017-06-30 18:06 ` Florian Weimer
2017-07-03 20:40 ` Andreas Schwab
0 siblings, 2 replies; 8+ messages in thread
From: Florian Weimer @ 2017-06-29 18:35 UTC (permalink / raw)
To: GNU C Library; +Cc: Andreas Schwab, Carlos O'Donell
[-- Attachment #1: Type: text/plain, Size: 318 bytes --]
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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: res_send-ns.patch --]
[-- Type: text/x-patch; name="res_send-ns.patch", Size: 5634 bytes --]
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)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] resolv: Do not copy IPv4 addresses to IPv6 address array
2017-06-29 18:35 [PATCH] resolv: Do not copy IPv4 addresses to IPv6 address array Florian Weimer
@ 2017-06-30 18:06 ` Florian Weimer
2017-07-03 20:40 ` Andreas Schwab
1 sibling, 0 replies; 8+ messages in thread
From: Florian Weimer @ 2017-06-30 18:06 UTC (permalink / raw)
To: GNU C Library; +Cc: Andreas Schwab, Carlos O'Donell
[-- Attachment #1: Type: text/plain, Size: 424 bytes --]
On 06/29/2017 08:35 PM, Florian Weimer wrote:
> 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?
This is the patch rebased to current master.
Thanks,
Florian
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: res_send-ns-2.patch --]
[-- Type: text/x-patch; name="res_send-ns-2.patch", Size: 5679 bytes --]
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 5d8b2c9..8f7b8dc 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 a7daae8..6ec957f 100644
--- a/resolv/res_send.c
+++ b/resolv/res_send.c
@@ -423,54 +423,6 @@ __libc_res_nsend(res_state statp, const u_char *buf, int buflen,
gotsomewhere = 0;
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 (unsigned int 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 (unsigned int 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;
- }
-
/* Name server index offset. Used to implement
RES_ROTATE. */
unsigned int ns_offset = nameserver_offset (statp);
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)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] resolv: Do not copy IPv4 addresses to IPv6 address array
2017-06-29 18:35 [PATCH] resolv: Do not copy IPv4 addresses to IPv6 address array Florian Weimer
2017-06-30 18:06 ` Florian Weimer
@ 2017-07-03 20:40 ` Andreas Schwab
2017-07-04 9:47 ` Florian Weimer
1 sibling, 1 reply; 8+ messages in thread
From: Andreas Schwab @ 2017-07-03 20:40 UTC (permalink / raw)
To: Florian Weimer; +Cc: GNU C Library, Carlos O'Donell
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.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] resolv: Do not copy IPv4 addresses to IPv6 address array
2017-07-03 20:40 ` Andreas Schwab
@ 2017-07-04 9:47 ` Florian Weimer
2017-07-04 12:56 ` Carlos O'Donell
0 siblings, 1 reply; 8+ messages in thread
From: Florian Weimer @ 2017-07-04 9:47 UTC (permalink / raw)
To: Andreas Schwab; +Cc: GNU C Library, Carlos O'Donell
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] resolv: Do not copy IPv4 addresses to IPv6 address array
2017-07-04 9:47 ` Florian Weimer
@ 2017-07-04 12:56 ` Carlos O'Donell
2017-07-04 16:53 ` Andreas Schwab
0 siblings, 1 reply; 8+ messages in thread
From: Carlos O'Donell @ 2017-07-04 12:56 UTC (permalink / raw)
To: Florian Weimer, Andreas Schwab; +Cc: GNU C Library
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.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] resolv: Do not copy IPv4 addresses to IPv6 address array
2017-07-04 12:56 ` Carlos O'Donell
@ 2017-07-04 16:53 ` Andreas Schwab
2017-07-04 18:05 ` Carlos O'Donell
0 siblings, 1 reply; 8+ messages in thread
From: Andreas Schwab @ 2017-07-04 16:53 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: Florian Weimer, GNU C Library
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.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] resolv: Do not copy IPv4 addresses to IPv6 address array
2017-07-04 16:53 ` Andreas Schwab
@ 2017-07-04 18:05 ` Carlos O'Donell
2017-07-27 9:51 ` Florian Weimer
0 siblings, 1 reply; 8+ messages in thread
From: Carlos O'Donell @ 2017-07-04 18:05 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Florian Weimer, GNU C Library
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.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] resolv: Do not copy IPv4 addresses to IPv6 address array
2017-07-04 18:05 ` Carlos O'Donell
@ 2017-07-27 9:51 ` Florian Weimer
0 siblings, 0 replies; 8+ messages in thread
From: Florian Weimer @ 2017-07-27 9:51 UTC (permalink / raw)
To: Carlos O'Donell, Andreas Schwab; +Cc: GNU C Library
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-07-27 9:49 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-29 18:35 [PATCH] resolv: Do not copy IPv4 addresses to IPv6 address array Florian Weimer
2017-06-30 18:06 ` Florian Weimer
2017-07-03 20:40 ` Andreas Schwab
2017-07-04 9:47 ` Florian Weimer
2017-07-04 12:56 ` Carlos O'Donell
2017-07-04 16:53 ` Andreas Schwab
2017-07-04 18:05 ` Carlos O'Donell
2017-07-27 9:51 ` Florian Weimer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).