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
