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
