Hi,
I’ve reworked the patch.
No warnings whatsoever, wether using GCC or LLVM.
checkpatch is OK, no warnings either.
Buffers are secured and all loops exit naturally or with an index I’ve
checked against off-by-one.
I’ve made a custom loop to remove the brackets : I found it easier than
to do arithmetic around numerous strlen calls.
Also, I’ve declared the address buffer of with a size of 48 as it looks
that INET6_ADDRSTRLEN is sometime define’d as 48, sometimes as 46.
--
Mildis
Le 2015-10-23 01:02, Willy Tarreau a écrit :
Hi,
On Thu, Oct 22, 2015 at 08:26:01PM +0200, Mildis wrote:
Hi Willy,
Here are two patches, rebased on current HEAD.
The first allows IPv6 with square brackets, the second parses colon to
look for a port.
OK thanks.
The logic is meant to be backward compatible with current behavior :
as
you suggested, starting from the end, it looks for either a colon or a
closing bracket.
If a colon is found, it???s considered as a port separator, thus
behaving
like previously.
If something else is found, either a closing bracket or nothing at
all,
then it consider that no port were provided.
str2ip2 is then in charge of validating wether the string is a valid
IP
address or not.
Malformed IP addresses, whatever its format, will be rejected.
Description looks good. However I'm still seeing mistakes in the
patches :
Subject: [PATCH 1/2] 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' };
There's no point initializing it here since you'll overwrite it later.
+ /* point to str by default, if FQDN processing is required */
+ char *tmpipptr = (char *)str;
This is strictly forbidden, never cast a const char * into a char *,
that's the best way to modify it without even getting a warning. Just
use a const char * for tmpipptr instead.
+ /* 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);
This last line must have emitted a warning, please do not send patches
which add build warnings, fix them before sending. A warning is very
often an indication that something which has not yet broken will soon
do.
You have a bug here, strncpy() can fill the whole string, and strlen()
will read past end. Always write a \0 on the last char after strncpy()
to enforce truncation (or reject too large inputs).
+ if (tmpip[iplength - 1] != ']') {
Here you still forget to check that iplength is > 0 so you can
actually read tmpip[-1] if the string I pass to your parser is
simply "[".
Also, I find it strange that you first copy the string and then
try to parse it instead of doing the opposite. It's much easier
since you even need to know the length for the strncpy() anyway.
(...)
@@ -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.
This comment doesn't belong to this patch since the feature is brought
by
the next patch (at least that's my understanding).
(..)
Subject: [PATCH 2/2] OPTIM: config: make last colon optional for IPv6
literal
square-bracketed notation
---
src/standard.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/src/standard.c b/src/standard.c
index 2e064b5..e93375c 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -879,12 +879,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
- port1 = "";
-
+ /* search for : or ] whatever comes first */
+ char *end = str2 + strlen(str2);
+ char *chr = end-1;
+ for (; chr >= str2; chr--) {
+ if (*chr == ']' || *chr == ':')
+ break;
+ }
+
+ if (*chr == ':') {
Here you can read before str2 if you never found a colon nor bracket
above, so not only you may dereference non-existing memory, but you
could actually find random data there including a colon!
+ /* Found a colon before a closing-bracket, must be a port
separator.
+ * This guarantee backward compatibility.
+ */
+ *chr++ = '\0';
+ port1 = chr;
+ }
+ else {
+ /* Either no colon and no closing-bracket
+ * or directly ending with a closing-bracket.
+ * However, no port */
+ port1 = "";
+ }
+
Also, please read the CONTRIBUTING file and coding-style.txt, you have
several mistakes there, among which :
- incorrect indent size, as you can see by yourself with your code
being significantly on the left from original code. You should use
tabs to indent and spaces to align.
- group the variable assignments together, skip a line then write the
code.
- please adjust the patches' subject. This is not an optim as it's
not
designed to improve performance. It's a minor or medium change. I'd
say the benefit is minor but for now the risks are still medium, as
seen above :-/
Thanks!
Willy
From 8ac634f6b90c6ccdd0293c1695cda7936aaba836 Mon Sep 17 00:00:00 2001
From: mildis <[email protected]>
Date: Mon, 26 Oct 2015 18:50:08 +0100
Subject: [PATCH] MINOR: config: allow IPv6 bracketed literals
---
src/standard.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 78 insertions(+), 15 deletions(-)
diff --git a/src/standard.c b/src/standard.c
index b5f4bf4..04917e4 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -620,9 +620,53 @@ const char *invalid_domainchar(const char *name) {
struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, int resolve)
{
struct hostent *he;
+ /* max IPv6 length, including brackets and terminating NULL */
+ char tmpip[48];
+ /* point to str by default, if FQDN processing is required */
+ const char *tmpipptr;
+
+ /* check IPv6 with square brackets */
+ if (str[0] == '[') {
+ size_t iplength = strlen(str);
+
+ if (iplength < 4) {
+ /* minimal size is 4 when using brackets "[::]" */
+ goto fail;
+ }
+ else if (iplength >= sizeof(tmpip)) {
+ /* IPv6 literal can not be larger than tmpip */
+ goto fail;
+ }
+ else {
+ if (str[iplength - 1] != ']') {
+ /* if address started with bracket, it should end with bracket */
+ goto fail;
+ }
+ else {
+ int srcidx, dstidx;
+
+ /* copy and strip the brackets */
+ for (srcidx = 1, dstidx = 0; dstidx < 47; srcidx++, dstidx++) {
+ if (str[srcidx] == ']')
+ break;
+ tmpip[dstidx] = str[srcidx];
+ }
+
+ /* secure buffer */
+ for (; dstidx <= 47; dstidx++)
+ tmpip[dstidx] = '\0';
+
+ /* switch to tmpip as we are working on a literal bracketed IPv6 */
+ tmpipptr = tmpip;
+ }
+ }
+ }
+ else {
+ tmpipptr = str;
+ }
/* 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 +675,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 +683,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 +698,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 +712,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 +733,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 +791,12 @@ 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. When using
+ * square brackets for IPv6 addresses, the port separator (colon) is optional.
+ * If not using square brackets, and 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).
@@ -855,12 +901,29 @@ struct sockaddr_storage *str2sa_range(const char *str, int *low, int *high, char
}
else { /* IPv4 and IPv6 */
int use_fqdn = 0;
+ char *end = str2 + strlen(str2);
+ char *chr;
- port1 = strrchr(str2, ':');
- if (port1)
- *port1++ = '\0';
- else
+ /* search for : or ] whatever comes first */
+ for (chr = end-1; chr > str2; chr--) {
+ if (*chr == ']' || *chr == ':')
+ break;
+ }
+
+ if (*chr == ':') {
+ /* Found a colon before a closing-bracket, must be a port separator.
+ * This guarantee backward compatibility.
+ */
+ *chr++ = '\0';
+ port1 = chr;
+ }
+ else {
+ /* Either no colon and no closing-bracket
+ * or directly ending with a closing-bracket.
+ * However, no port.
+ */
port1 = "";
+ }
if (str2ip2(str2, &ss, 0) == NULL) {
use_fqdn = 1;
--
2.4.9 (Apple Git-60)
