Hi again,
Let’s go step by step.
Here is a first patch which allows square-brackets literals.
It does not change the port parser, just remove the brackets in str2ip2.
It expects the IP address to be either plain or surrounded with square
brackets, thus if first and last chars are not respectively opening and
closing square brackets, it fails.
I’m still trying to figure a good implementation of the port parser to
make the colon optional for IPv6 addresses.
Regards,
--
Mildis
Le 2015-10-10 15:49, Willy Tarreau a écrit :
Hi,
On Sat, Oct 10, 2015 at 01:50:46PM +0200, Mildis wrote:
Here is a working patch for IPv6 literal with square brackets.
Tested with :
"2001:db8::1234:5678",
"2001:db8::1234:5678:",
"2001:db8::1234:5678:80",
"2001:db8::1234:5678:80:",
"::",
":::",
":::80",
"[2001:db8::1234:5678]",
"[2001:db8::1234:5678]:",
"[2001:db8::1234:5678]:80",
"[::]",
"[::]:",
"[::]:80"
There are several oddities and/or bugs in your implementation, please
look below :
diff --git a/src/standard.c b/src/standard.c
index b5f4bf4..7b49332 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -622,7 +622,7 @@ struct sockaddr_storage *str2ip2(const char *str,
struct sockaddr_storage *sa, i
struct hostent *he;
/* Any IPv6 address */
- if (str[0] == ':' && str[1] == ':' && !str[2]) {
+ if (str[0] == ':' && str[1] == ':' && (str[2]=='\0' || !str[2])) {
Here you have duplicated the same test on str[2], I don't know what
your original intent was but it's definitely not what you've done.
if (!sa->ss_family || sa->ss_family == AF_UNSPEC)
sa->ss_family = AF_INET6;
else if (sa->ss_family != AF_INET6)
@@ -637,11 +637,38 @@ struct sockaddr_storage *str2ip2(const char
*str, struct sockaddr_storage *sa, i
return sa;
}
+ /* check IPv6 literal */
+ if (str[0] == '[') {
+ if (strrchr(str, ']') != NULL)
Just a hint, strrchr() is more expensive than strchr() since it needs
to
walk till the end, so you should only use it when you have no other
option.
Here it's useless since you're only checking whether you find a closing
bracket or not.
+ /* has opening AND closing bracket */
+ sa->ss_family = AF_INET6;
+ else
+ /* no closing bracket */
+ goto fail;
+ }
+
/* check for IPv6 first */
- if ((!sa->ss_family || sa->ss_family == AF_UNSPEC || sa->ss_family
== AF_INET6) &&
- inet_pton(AF_INET6, str, &((struct sockaddr_in6
*)sa)->sin6_addr)) {
- sa->ss_family = AF_INET6;
- return sa;
+ if (!sa->ss_family || sa->ss_family == AF_UNSPEC || sa->ss_family ==
AF_INET6) {
+ /* IPv6 literal with opening and closing bracket ? */
+ if (str[0] == '[' && strchr(str, ']') != NULL) {
Here you're doing the same check again as you did above, it would have
been
better to save the result of the initial test and reuse it here.
+ /* strip the closing bracket */
+ char *tmpip6 = strdup(str);
You forgot to free() this strdup so you end up with a memory leak for
each
address that you're parsing. Also, there's no point allocating then
freeing
memory just to parse an address, an IPv6 address has a fixed maximum
length
which is INET6_ADDRSTRLEN, so better declare a local variable of this
size
(+1) and copy the string there. It also saves malloc checks.
+ if (tmpip6) {
+ *(strchr(tmpip6, ']')) = '\0';
Hint, it's the third time you look for this bracket :-)
Also this time it kills the first closing bracket, thus you don't know
if there's anything after it, which is a problem because it means that
this address will parse without error as 2001:db8::, ignoring the 1234
part and keeping 5678 as the port :
[2001:db8::]:1234:5678
Better check at the begining if there's anything past the first closing
bracket and report an error.
+ /* if inet_pton is OK, it is a valid IPv6
address */
+ if (inet_pton(AF_INET6, tmpip6+1, &((struct
sockaddr_in6
*)sa)->sin6_addr)) {
+ sa->ss_family = AF_INET6;
+ return sa;
+ }
+ }
+ else
+ goto fail;
+ }
+ /* IPv6 without brackets */
+ else if (inet_pton(AF_INET6, str, &((struct sockaddr_in6
*)sa)->sin6_addr)) {
+ sa->ss_family = AF_INET6;
+ return sa;
+ }
Given the number of different paths to inet_pton() I guess it would be
simpler to just move str to where the actual address to be parsed is
(eg:
either str or tmpip6) and fall back to the same call.
}
/* then check for IPv4 */
@@ -747,10 +774,10 @@ struct sockaddr_storage *str2ip2(const char
*str, struct sockaddr_storage *sa, i
* the first byte of the address.
* - "fd@" => an integer must follow, and is a file descriptor
number.
*
- * Also note that in order to avoid any ambiguity with IPv6 addresses,
the ':'
- * is mandatory after the IP address even when no port is specified.
NULL is
- * returned if the address cannot be parsed. The <low> and <high>
ports are
- * always initialized if non-null, even for non-IP families.
+ * IPv6 addresses can be declared with or with square brackets. If not
using
+ * square brackets, the last colon ':' is mandatory even when no port
is
+ * specified. NULL is returned if the address cannot be parsed. The
<low> and
+ * <high> ports are always initialized if non-null, even for non-IP
families.
*
* If <pfx> is non-null, it is used as a string prefix before any
path-based
* address (typically the path to a unix socket).
@@ -856,11 +883,28 @@ struct sockaddr_storage *str2sa_range(const char
*str, int *low, int *high, char
else { /* IPv4 and IPv6 */
int use_fqdn = 0;
- port1 = strrchr(str2, ':');
- if (port1)
- *port1++ = '\0';
- else
+ /* IPv6 wildcard */
+ if (!strcmp(str2, "::")) {
+ port1 = "";
+ }
You're changing the parser here, because it now accepts "::" in a place
where a port was expected.
+ /* IPv6 literal with a port */
+ else if (strstr(str2, "]:")) {
Please NEVER EVER use strstr() in a parser, it is ALWAYS wrong. It
matches
crap at any random place and it is the best way to introduce bugs that
are
hard to fix afterwards without breaking deployed configs because people
were passing through the cracks without noticing.
+ port1 = strrchr(str2, ':');
+ if (port1)
+ *port1++ = '\0';
+ }
+ /* IPv6 literal without a port */
+ else if (strstr(str2, "]\0")) {
Here there's a bug as well. you call will simply look for "]" at any
random place since "\0" is the end of the pattern string. For example
it will easily match "]12345". So this test is wrong as well.
port1 = "";
+ }
+ /* not an IPv6 in square bracket */
+ else if (!strchr(str2, '[') && !strrchr(str2, ']')) {
+ port1 = strrchr(str2, ':');
+ if (port1)
+ *port1++ = '\0';
+ else
+ port1 = "";
+ }
And here I'm not sure how you deal with inconsistent addresses
containing
just one closing bracket or even addresses looking like this : "]1234["
since they will match the test above.
In my opinion, there are two possible approaches here. Either the
parser
considers that it uses the part under brackets as the IP address and
uses whatever else as the port (which must then parse correctly or
fail),
or the parser needs to figure whether there is a port after a closing
bracket or not, then consider the bracket as an isolated address.
I tend to think the second method should be simpler and would have less
impact on existing code though I could be wrong :
end = str2 + strlen(str2);
chr = end - 1;
while (chr >= str2) {
chr--;
if (*chr == ']' || *chr == ':')
break;
}
if (*chr == ':') {
/* we have a port here, fall back to existing code to
* parse the port. Address is between str2 and chr-1.
*/
}
else if (*chr == ']') {
/* bracket after the last colon */
if (end > chr + 1) {
/* error, only a ':' can follow the closing bracket */
}
/* else fine, no port, address is between str2 and chr */
}
The first method would be a bit different, faster but maybe a bit
trickier, it relies on the fact that the opening bracket can only
be the first character, and it also overlaps with what str2ip2 has
to do :
if (*str2 == '[' ) {
chr = strchr(str2, ']');
if (!chr) {
/* error, missing closing bracket */
}
if (chr[1] && chr[1] != ':') {
/* error, forbidden char after bracket */
}
if (chr[1] && strchr(chr + 2, ':')) {
/* error, extra colon after the bracket */
}
/* here you're certain that if there is something
* after the first closing bracket, it's a colon
* and it'sthe last colon, so what follows is
* necessarily the port. If an extra closing
* bracket appears there, the port parser will
* catch it. Eg: [2001:db8::1234]:5678]
*/
}
The you go on with the current parser which looks for the port using
strrchr(':') and let it call str2ip2() on the part between str2 and
the colon or end of string.
Still I tend to prefer this last approach as it more naturally flows
like we read the addresses.
Regards,
Willy
From 58dced2c5117f01ba2bd65e0aa0d41f452e39fb5 Mon Sep 17 00:00:00 2001
From: mildis <[email protected]>
Date: Sat, 17 Oct 2015 15:38:51 +0200
Subject: [PATCH] OPTIM: config: allow square-bracketed literal IPv6.
To enhance readability, allows IPv6 literal addresses with square brackets.
Last colon is still mandatory for address/port separation.
---
src/standard.c | 45 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/src/standard.c b/src/standard.c
index b5f4bf4..2e064b5 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -620,9 +620,31 @@ const char *invalid_domainchar(const char *name) {
struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, int resolve)
{
struct hostent *he;
+ /* add 2 chars for square brackets */
+ char tmpip[INET6_ADDRSTRLEN+2] = { '\0' };
+ /* point to str by default, if FQDN processing is required */
+ char *tmpipptr = (char *)str;
+
+ /* check IPv6 with square brackets */
+ if (str[0] == '[') {
+ /* copy and strip the opening bracket */
+ strncpy(tmpip, str+1, sizeof(tmpip));
+ size_t iplength = strlen(tmpip);
+ if (tmpip[iplength - 1] != ']') {
+ /* if address started with bracket,
+ it should end with bracket */
+ goto fail;
+ }
+ else {
+ /* switch to tmpip as we are working on a literal bracketed IPv6 */
+ tmpipptr = tmpip;
+ /* strip last bracket */
+ tmpipptr[iplength-1] = '\0';
+ }
+ }
/* Any IPv6 address */
- if (str[0] == ':' && str[1] == ':' && !str[2]) {
+ if (tmpipptr[0] == ':' && tmpipptr[1] == ':' && !tmpipptr[2]) {
if (!sa->ss_family || sa->ss_family == AF_UNSPEC)
sa->ss_family = AF_INET6;
else if (sa->ss_family != AF_INET6)
@@ -631,7 +653,7 @@ struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, i
}
/* Any address for the family, defaults to IPv4 */
- if (!str[0] || (str[0] == '*' && !str[1])) {
+ if (!tmpipptr[0] || (tmpipptr[0] == '*' && !tmpipptr[1])) {
if (!sa->ss_family || sa->ss_family == AF_UNSPEC)
sa->ss_family = AF_INET;
return sa;
@@ -639,14 +661,14 @@ struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, i
/* check for IPv6 first */
if ((!sa->ss_family || sa->ss_family == AF_UNSPEC || sa->ss_family == AF_INET6) &&
- inet_pton(AF_INET6, str, &((struct sockaddr_in6 *)sa)->sin6_addr)) {
+ inet_pton(AF_INET6, tmpipptr, &((struct sockaddr_in6 *)sa)->sin6_addr)) {
sa->ss_family = AF_INET6;
return sa;
}
/* then check for IPv4 */
if ((!sa->ss_family || sa->ss_family == AF_UNSPEC || sa->ss_family == AF_INET) &&
- inet_pton(AF_INET, str, &((struct sockaddr_in *)sa)->sin_addr)) {
+ inet_pton(AF_INET, tmpipptr, &((struct sockaddr_in *)sa)->sin_addr)) {
sa->ss_family = AF_INET;
return sa;
}
@@ -654,7 +676,7 @@ struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, i
if (!resolve)
return NULL;
- if (!dns_hostname_validation(str, NULL))
+ if (!dns_hostname_validation(tmpipptr, NULL))
return NULL;
#ifdef USE_GETADDRINFO
@@ -668,7 +690,7 @@ struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, i
hints.ai_flags = 0;
hints.ai_protocol = 0;
- if (getaddrinfo(str, NULL, &hints, &result) == 0) {
+ if (getaddrinfo(tmpipptr, NULL, &hints, &result) == 0) {
if (!sa->ss_family || sa->ss_family == AF_UNSPEC)
sa->ss_family = result->ai_family;
else if (sa->ss_family != result->ai_family)
@@ -689,7 +711,7 @@ struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, i
}
#endif
/* try to resolve an IPv4/IPv6 hostname */
- he = gethostbyname(str);
+ he = gethostbyname(tmpipptr);
if (he) {
if (!sa->ss_family || sa->ss_family == AF_UNSPEC)
sa->ss_family = he->h_addrtype;
@@ -747,10 +769,11 @@ struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, i
* the first byte of the address.
* - "fd@" => an integer must follow, and is a file descriptor number.
*
- * Also note that in order to avoid any ambiguity with IPv6 addresses, the ':'
- * is mandatory after the IP address even when no port is specified. NULL is
- * returned if the address cannot be parsed. The <low> and <high> ports are
- * always initialized if non-null, even for non-IP families.
+ * IPv6 addresses can be declared with or without square brackets. Also note
+ * that in order to avoid any ambiguity with IPv6 addresses, the last colon ':'
+ * is mandatory even when no port is specified. NULL is returned if the address
+ * cannot be parsed. The <low> and <high> ports are always initialized if
+ * non-null, even for non-IP families.
*
* If <pfx> is non-null, it is used as a string prefix before any path-based
* address (typically the path to a unix socket).
--
2.3.8 (Apple Git-58)
