* xtables: various fixes for handling multiple src/dst addresses
@ 2011-02-22 0:10 Wes Campaigne
2011-02-22 0:10 ` [PATCH 1/4] xtables: use *all* IPv6 addresses resolved from a hostname Wes Campaigne
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Wes Campaigne @ 2011-02-22 0:10 UTC (permalink / raw
To: netfilter-devel
Hi! I've been working on enabling robust IPv6 support in the TomatoUSB router firmware. In the process, I found and fixed a few old iptables bugs, mostly related to lookups and the handling of multiple addresses. Please review!
NOTE: TomatoUSB is actually still using an old 1.3.8 branch of iptables, and analogous versions of these patches against that version have been commited on the TomatoUSB repo at repo.or.cz/w/tomato.git tomato-RT
===
Wes Campaigne (4):
xtables: use *all* IPv6 addresses resolved from a hostname
xtables: fix excessive memory allocation in host_to_ipaddr
xtables: fix the broken detection/removal of redundant addresses
xtables: use the correct loop count when applying masks to addresses
xtables.c | 45 ++++++++++++++++++++-------------------------
1 files changed, 20 insertions(+), 25 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4] xtables: use *all* IPv6 addresses resolved from a hostname
2011-02-22 0:10 xtables: various fixes for handling multiple src/dst addresses Wes Campaigne
@ 2011-02-22 0:10 ` Wes Campaigne
2011-02-22 0:10 ` [PATCH 2/4] xtables: fix excessive memory allocation in host_to_ipaddr Wes Campaigne
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Wes Campaigne @ 2011-02-22 0:10 UTC (permalink / raw
To: netfilter-devel
Fixes a long-standing issue where host_to_ip6addr would only ever
examine/return the only the first item of the address chain
returned by getaddrinfo, instead of traversing the chain and
copying each of them.
This has always been how host_to_ipaddr behaves, and all of the other
related ipv6 code is already written to handle multiple possible addresses.
Signed-off-by: Wes Campaigne <westacular@gmail.com>
---
xtables.c | 29 +++++++++++------------------
1 files changed, 11 insertions(+), 18 deletions(-)
diff --git a/xtables.c b/xtables.c
index 57d5d13..aea32ca 100644
--- a/xtables.c
+++ b/xtables.c
@@ -1415,17 +1415,17 @@ struct in6_addr *xtables_numeric_to_ip6addr(const char *num)
static struct in6_addr *
host_to_ip6addr(const char *name, unsigned int *naddr)
{
- static struct in6_addr *addr;
+ struct in6_addr *addr;
struct addrinfo hints;
struct addrinfo *res;
+ struct addrinfo *p;
int err;
+ unsigned int i;
memset(&hints, 0, sizeof(hints));
hints.ai_flags = AI_CANONNAME;
hints.ai_family = AF_INET6;
hints.ai_socktype = SOCK_RAW;
- hints.ai_protocol = IPPROTO_IPV6;
- hints.ai_next = NULL;
*naddr = 0;
if ((err = getaddrinfo(name, NULL, &hints, &res)) != 0) {
@@ -1434,20 +1434,19 @@ host_to_ip6addr(const char *name, unsigned int *naddr)
#endif
return NULL;
} else {
- if (res->ai_family != AF_INET6 ||
- res->ai_addrlen != sizeof(struct sockaddr_in6))
- return NULL;
-
+ /* Find length of address-chain */
+ for(p = res; p != NULL; p = p->ai_next)
+ (*naddr)++;
#ifdef DEBUG
fprintf(stderr, "resolved: len=%d %s ", res->ai_addrlen,
xtables_ip6addr_to_numeric(&((struct sockaddr_in6 *)res->ai_addr)->sin6_addr));
#endif
- /* Get the first element of the address-chain */
- addr = xtables_malloc(sizeof(struct in6_addr));
- memcpy(addr, &((const struct sockaddr_in6 *)res->ai_addr)->sin6_addr,
- sizeof(struct in6_addr));
+ /* Copy each element of the address-chain */
+ addr = xtables_calloc(*naddr, sizeof(struct in6_addr));
+ for(p = res, i = 0; p != NULL && i < *naddr; p = p->ai_next, i++)
+ memcpy(&addr[i], &((const struct sockaddr_in6 *)p->ai_addr)->sin6_addr,
+ sizeof(struct in6_addr));
freeaddrinfo(res);
- *naddr = 1;
return addr;
}
@@ -1559,12 +1558,6 @@ xtables_ip6parse_multiple(const char *name, struct in6_addr **addrpp,
strcpy(buf, "::");
addrp = ip6parse_hostnetwork(buf, &n);
- /* ip6parse_hostnetwork only ever returns one IP
- address (it exits if the resolution fails).
- Therefore, n will always be 1 here. Leaving the
- code below in anyway in case ip6parse_hostnetwork
- is improved some day to behave like
- ipparse_hostnetwork: */
if (n > 1) {
count += n - 1;
*addrpp = xtables_realloc(*addrpp,
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/4] xtables: fix excessive memory allocation in host_to_ipaddr
2011-02-22 0:10 xtables: various fixes for handling multiple src/dst addresses Wes Campaigne
2011-02-22 0:10 ` [PATCH 1/4] xtables: use *all* IPv6 addresses resolved from a hostname Wes Campaigne
@ 2011-02-22 0:10 ` Wes Campaigne
2011-02-22 0:10 ` [PATCH 3/4] xtables: fix the broken detection/removal of redundant addresses Wes Campaigne
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Wes Campaigne @ 2011-02-22 0:10 UTC (permalink / raw
To: netfilter-devel
host_to_ipaddr was unnecessarily asking for an array of length n^2
to store just n addresses.
Signed-off-by: Wes Campaigne <westacular@gmail.com>
---
xtables.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/xtables.c b/xtables.c
index aea32ca..9a51e81 100644
--- a/xtables.c
+++ b/xtables.c
@@ -1143,7 +1143,7 @@ static struct in_addr *host_to_ipaddr(const char *name, unsigned int *naddr)
while (host->h_addr_list[*naddr] != NULL)
++*naddr;
- addr = xtables_calloc(*naddr, sizeof(struct in_addr) * *naddr);
+ addr = xtables_calloc(*naddr, sizeof(struct in_addr));
for (i = 0; i < *naddr; i++)
memcpy(&addr[i], host->h_addr_list[i],
sizeof(struct in_addr));
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/4] xtables: fix the broken detection/removal of redundant addresses
2011-02-22 0:10 xtables: various fixes for handling multiple src/dst addresses Wes Campaigne
2011-02-22 0:10 ` [PATCH 1/4] xtables: use *all* IPv6 addresses resolved from a hostname Wes Campaigne
2011-02-22 0:10 ` [PATCH 2/4] xtables: fix excessive memory allocation in host_to_ipaddr Wes Campaigne
@ 2011-02-22 0:10 ` Wes Campaigne
2011-02-22 0:10 ` [PATCH 4/4] xtables: use the correct loop count when applying masks to addresses Wes Campaigne
2011-02-22 3:48 ` xtables: various fixes for handling multiple src/dst addresses Jan Engelhardt
4 siblings, 0 replies; 6+ messages in thread
From: Wes Campaigne @ 2011-02-22 0:10 UTC (permalink / raw
To: netfilter-devel
This same block of code, apparently to detect if addresses are
identical after applying the mask, and to skip the duplicates and
the ones made redundant by the mask, has been present and unchanged
from as far back as I could find (circa iptables 1.2).
By inspection, it was wrong, and always has been: once the code
finds a duplicate, it will drop the rest of the array one by one as
it re-detects the same duplicate over and over. When the addresses
came from a single hostname lookup, and their order was random,
then this created unpredictable behaviour by iptables, which seem
to ignore some of those addresses at random times.
I suspect the original idea also involved a swap between the
duplicate and the address from the (current) end of the array, but
a line of code to do that seems to have never existed. I've finally
added it. (Well, as much as is needed: there doesn't need to be a
full swap, because we're just going to ignore the duplicate,
pretend the array is one shorter, and never look at the contents of
the end again. So, we can get away with just copying from the end.)
Signed-off-by: Wes Campaigne <westacular@gmail.com>
---
xtables.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/xtables.c b/xtables.c
index 9a51e81..c96efa0 100644
--- a/xtables.c
+++ b/xtables.c
@@ -1306,13 +1306,14 @@ void xtables_ipparse_any(const char *name, struct in_addr **addrpp,
strcpy(buf, "0.0.0.0");
addrp = *addrpp = ipparse_hostnetwork(buf, naddrs);
+ /* Shuffle from end of array to remove redundant addresses */
n = *naddrs;
for (i = 0, j = 0; i < n; ++i) {
addrp[j++].s_addr &= maskp->s_addr;
for (k = 0; k < j - 1; ++k)
if (addrp[k].s_addr == addrp[j-1].s_addr) {
- --*naddrs;
- --j;
+ memcpy(&addrp[--j], &addrp[--*naddrs],
+ sizeof(struct in_addr));
break;
}
}
@@ -1608,6 +1609,7 @@ void xtables_ip6parse_any(const char *name, struct in6_addr **addrpp,
strcpy(buf, "::");
addrp = *addrpp = ip6parse_hostnetwork(buf, naddrs);
+ /* Shuffle from end of array to remove redundant addresses */
n = *naddrs;
for (i = 0, j = 0; i < n; ++i) {
for (k = 0; k < 4; ++k)
@@ -1615,8 +1617,8 @@ void xtables_ip6parse_any(const char *name, struct in6_addr **addrpp,
++j;
for (k = 0; k < j - 1; ++k)
if (IN6_ARE_ADDR_EQUAL(&addrp[k], &addrp[j - 1])) {
- --*naddrs;
- --j;
+ memcpy(&addrp[--j], &addrp[--*naddrs],
+ sizeof(struct in_addr));
break;
}
}
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 4/4] xtables: use the correct loop count when applying masks to addresses
2011-02-22 0:10 xtables: various fixes for handling multiple src/dst addresses Wes Campaigne
` (2 preceding siblings ...)
2011-02-22 0:10 ` [PATCH 3/4] xtables: fix the broken detection/removal of redundant addresses Wes Campaigne
@ 2011-02-22 0:10 ` Wes Campaigne
2011-02-22 3:48 ` xtables: various fixes for handling multiple src/dst addresses Jan Engelhardt
4 siblings, 0 replies; 6+ messages in thread
From: Wes Campaigne @ 2011-02-22 0:10 UTC (permalink / raw
To: netfilter-devel
'n' is a subtotal from the inner loop; 'count' is the total number
of addrs/masks. This problem wasn't merely cosmetic -- rules where
the address has not been zeroed in accordance with the mask are
silently ignored by netfilter.
Signed-off-by: Wes Campaigne <westacular@gmail.com>
---
xtables.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/xtables.c b/xtables.c
index c96efa0..f872754 100644
--- a/xtables.c
+++ b/xtables.c
@@ -1272,7 +1272,7 @@ void xtables_ipparse_multiple(const char *name, struct in_addr **addrpp,
free(addrp);
}
*naddrs = count;
- for (i = 0; i < n; ++i)
+ for (i = 0; i < count; ++i)
(*addrpp+i)->s_addr &= (*maskpp+i)->s_addr;
}
@@ -1581,7 +1581,7 @@ xtables_ip6parse_multiple(const char *name, struct in6_addr **addrpp,
free(addrp);
}
*naddrs = count;
- for (i = 0; i < n; ++i)
+ for (i = 0; i < count; ++i)
for (j = 0; j < 4; ++j)
(*addrpp+i)->s6_addr32[j] &= (*maskpp+i)->s6_addr32[j];
}
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: xtables: various fixes for handling multiple src/dst addresses
2011-02-22 0:10 xtables: various fixes for handling multiple src/dst addresses Wes Campaigne
` (3 preceding siblings ...)
2011-02-22 0:10 ` [PATCH 4/4] xtables: use the correct loop count when applying masks to addresses Wes Campaigne
@ 2011-02-22 3:48 ` Jan Engelhardt
4 siblings, 0 replies; 6+ messages in thread
From: Jan Engelhardt @ 2011-02-22 3:48 UTC (permalink / raw
To: Wes Campaigne; +Cc: netfilter-devel
On Tuesday 2011-02-22 01:10, Wes Campaigne wrote:
>Hi! I've been working on enabling robust IPv6 support in the TomatoUSB router firmware. In the process, I found and fixed a few old iptables bugs, mostly related to lookups and the handling of multiple addresses. Please review!
>
>NOTE: TomatoUSB is actually still using an old 1.3.8 branch of iptables, and analogous versions of these patches against that version have been commited on the TomatoUSB repo at repo.or.cz/w/tomato.git tomato-RT
>
>===
>Wes Campaigne (4):
> xtables: use *all* IPv6 addresses resolved from a hostname
> xtables: fix excessive memory allocation in host_to_ipaddr
> xtables: fix the broken detection/removal of redundant addresses
> xtables: use the correct loop count when applying masks to addresses
The last one has already beek taken care of (cf.
http://bugs.debian.org/611990), I am checking the rest as the
lines enlighten me :)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-02-22 3:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-02-22 0:10 xtables: various fixes for handling multiple src/dst addresses Wes Campaigne
2011-02-22 0:10 ` [PATCH 1/4] xtables: use *all* IPv6 addresses resolved from a hostname Wes Campaigne
2011-02-22 0:10 ` [PATCH 2/4] xtables: fix excessive memory allocation in host_to_ipaddr Wes Campaigne
2011-02-22 0:10 ` [PATCH 3/4] xtables: fix the broken detection/removal of redundant addresses Wes Campaigne
2011-02-22 0:10 ` [PATCH 4/4] xtables: use the correct loop count when applying masks to addresses Wes Campaigne
2011-02-22 3:48 ` xtables: various fixes for handling multiple src/dst addresses Jan Engelhardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.