[Openvpn-devel,v2] ifconfig-ipv6(-push): allow using hostnames

Message ID 20171202085438.2393-1-a@unstable.cc
State Superseded
Delegated to: Gert Doering
Headers show
Series
  • [Openvpn-devel,v2] ifconfig-ipv6(-push): allow using hostnames
Related show

Commit Message

Antonio Quartulli Dec. 2, 2017, 8:54 a.m.
Similarly to ifconfig(-push), its IPv6 counterpart is now able to
accept hostnames as well instead of IP addresses in numeric form.

Basically this means that the user is now allowed to specify
something like this:

ifconfig-ipv6-push my.hostname.cx/64

This is exactly the same behaviour that we already have with
ifconfig(-push).

The generic code introduced in this patch will be later used to
implement the /bits parsing support for IPv4 addresses.

Trac: #808
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---

v2:
- rebased on top of master
- style adapted to new CodingStyle

 src/openvpn/options.c |  61 ------------------------
 src/openvpn/options.h |   4 --
 src/openvpn/socket.c  | 126 +++++++++++++++++++++++++++++++++++++++++++++-----
 src/openvpn/socket.h  |  12 +++++
 4 files changed, 126 insertions(+), 77 deletions(-)

Comments

Selva Nair Dec. 2, 2017, 8:27 p.m. | #1
Hi,

On Sat, Dec 2, 2017 at 3:54 AM, Antonio Quartulli <a@unstable.cc> wrote:

> Similarly to ifconfig(-push), its IPv6 counterpart is now able to
> accept hostnames as well instead of IP addresses in numeric form.
>

If dns names currently work for ifconfig-push (I didn't know),  makes sense
to
support it for ipv6 as well.

But getaddrinfo can take a long time to timeout, so we are adding another
potentially blocking call on the server. Should not be an issue on a well
administered server, but just saying...

Otherwise looks good except for a couple of issues as below:


> Basically this means that the user is now allowed to specify
> something like this:
>
> ifconfig-ipv6-push my.hostname.cx/64
>
> This is exactly the same behaviour that we already have with
> ifconfig(-push).
>
> The generic code introduced in this patch will be later used to
> implement the /bits parsing support for IPv4 addresses.
>
> Trac: #808
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
>
> v2:
> - rebased on top of master
> - style adapted to new CodingStyle
>
>  src/openvpn/options.c |  61 ------------------------
>  src/openvpn/options.h |   4 --
>  src/openvpn/socket.c  | 126 ++++++++++++++++++++++++++++++
> +++++++++++++++-----
>  src/openvpn/socket.h  |  12 +++++
>  4 files changed, 126 insertions(+), 77 deletions(-)
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 8e5cdf7f..767cdaeb 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -1033,67 +1033,6 @@ get_ip_addr(const char *ip_string, int msglevel,
> bool *error)
>      return ret;
>  }
>
> -/* helper: parse a text string containing an IPv6 address + netbits
> - * in "standard format" (2001:dba::/32)
> - * "/nn" is optional, default to /64 if missing
> - *
> - * return true if parsing succeeded, modify *network and *netbits
> - */
> -bool
> -get_ipv6_addr( const char *prefix_str, struct in6_addr *network,
> -               unsigned int *netbits, int msglevel)
> -{
> -    char *sep, *endp;
> -    int bits;
> -    struct in6_addr t_network;
> -
> -    sep = strchr( prefix_str, '/' );
> -    if (sep == NULL)
> -    {
> -        bits = 64;
> -    }
> -    else
> -    {
> -        bits = strtol( sep+1, &endp, 10 );
> -        if (*endp != '\0' || bits < 0 || bits > 128)
> -        {
> -            msg(msglevel, "IPv6 prefix '%s': invalid '/bits' spec",
> prefix_str);
> -            return false;
> -        }
> -    }
> -
> -    /* temporary replace '/' in caller-provided string with '\0',
> otherwise
> -     * inet_pton() will refuse prefix string
> -     * (alternative would be to strncpy() the prefix to temporary buffer)
> -     */
> -
> -    if (sep != NULL)
> -    {
> -        *sep = '\0';
> -    }
> -
> -    if (inet_pton( AF_INET6, prefix_str, &t_network ) != 1)
> -    {
> -        msg(msglevel, "IPv6 prefix '%s': invalid IPv6 address",
> prefix_str);
> -        return false;
> -    }
> -
> -    if (sep != NULL)
> -    {
> -        *sep = '/';
> -    }
> -
> -    if (netbits != NULL)
> -    {
> -        *netbits = bits;
> -    }
> -    if (network != NULL)
> -    {
> -        *network = t_network;
> -    }
> -    return true;                /* parsing OK, values set */
> -}
> -
>  /**
>   * Returns newly allocated string containing address part without "/nn".
>   *
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index 035c6d15..d67c2785 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -817,8 +817,4 @@ void options_string_import(struct options *options,
>                             unsigned int *option_types_found,
>                             struct env_set *es);
>
> -bool get_ipv6_addr( const char *prefix_str, struct in6_addr *network,
> -                    unsigned int *netbits, int msglevel );
> -
> -
>  #endif /* ifndef OPTIONS_H */
> diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
> index 0fc91f21..4cadae23 100644
> --- a/src/openvpn/socket.c
> +++ b/src/openvpn/socket.c
> @@ -74,12 +74,102 @@ sf2gaf(const unsigned int getaddr_flags,
>  /*
>   * Functions related to the translation of DNS names to IP addresses.
>   */
> +static int
> +get_addr_generic(sa_family_t af, unsigned int flags, const char *hostname,
> +                 void *network, unsigned int *netbits,
> +                 int resolve_retry_seconds, volatile int *signal_received,
> +                 int msglevel)
> +{
> +    char *endp, *sep, *var_host;
> +    uint8_t bits, max_bits;
> +    struct addrinfo *ai;
> +    int ret = -1;
> +
> +    ASSERT(hostname);
> +
> +    /* assign family specific default values */
> +    switch (af)
> +    {
> +        case AF_INET:
> +            bits = 0;
> +            max_bits = sizeof(in_addr_t) * 8;
> +            break;
> +        case AF_INET6:
> +            bits = 64;
> +            max_bits = sizeof(struct in6_addr) * 8;
> +            break;
> +        default:
> +            ASSERT(0);
> +    }
> +
> +    /* we need to modify the hostname received as input, but we don't
> want to
> +     * touch it directly as it might be a constant string.
> +     *
> +     * Therefore, we clone the string here and free it at the end of the
> +     * function */
> +    var_host = strdup(hostname);
> +    ASSERT(var_host);
>

I think ASSERT should be used only to catch errors in coding logic,
not for plausible runtime errors like this. Especially since this happens
on the server, no reason to terminate the process here.

Instead, log an error (M_NONFTAL|M_ERRNO will do nicely) and return -1
or goto out. The option parser will log a generic warning, but still useful
to log here using M_ERRNO for more specific info.

Alternatively one could use a buffer on the stack --  easy to do as good
old dns names are limited in size (255 octets max?) and the current option
parser also limits the argument passed here to < 255 bytes. But if we
support
internationalized domain names (currently we do not, do we?) and if the line
length in option parser is ever increased, a much larger buffer would be
needed.


> +
> +    /* check if this hostname has a /bits suffix */
> +    sep = strchr(var_host , '/');
> +    if (sep)
> +    {
> +        bits = strtoul(sep + 1, &endp, 10);


There are a number of such type coercions in the patch
(ulong to uint8_t, size_t  to unit8_t etc.) that some compilers (aka MSVC:)
may warn about, but I do not personally care. All are safe and deliberate
except for the nitpicking below.

+        if ((*endp != '\0') || (bits > max_bits))


That (bits > max_bits) check will not catch many input errors, as the
input is  already truncated to uint8_t. For example, /255 will be flagged
as an error, but /256 will pass as 0.

Though its not possible to catch all input errors, keeping bits as unsigned
int (instead of uint8_t) may be better here. That'll  also match netbits
in the function signature, return value of strtoul etc..

+        {
> +            msg(msglevel, "IP prefix '%s': invalid '/bits' spec",
> hostname);
> +            goto out;
> +        }
> +        /* temporary truncate string at '/'. This allows the IP
> +         * parsing routines to properly work. Will be restored later.
> +         */
>

This comment is no longer required as there is no need to restore '/'
(also see below)


> +        *sep = '\0';
> +    }
> +
> +    ret = openvpn_getaddrinfo(flags & ~GETADDR_HOST_ORDER, var_host, NULL,
> +                              resolve_retry_seconds, signal_received, af,
> &ai);
> +    if ((ret == 0) && network)
> +    {
> +        struct in6_addr *ip6;
> +        in_addr_t *ip4;
> +
> +        switch (af)
> +        {
> +            case AF_INET:
> +                ip4 = network;
> +                *ip4 = ((struct sockaddr_in *)ai->ai_addr)->sin_addr.s_
> addr;
> +
> +                if (flags & GETADDR_HOST_ORDER)
> +                {
> +                    *ip4 = ntohl(*ip4);
> +                }
> +                break;
> +            case AF_INET6:
> +                ip6 = network;
> +                *ip6 = ((struct sockaddr_in6 *)ai->ai_addr)->sin6_addr;
> +                break;
> +            default:
> +                ASSERT(0);
> +        }
> +        freeaddrinfo(ai);
> +    }
> +
> +    if (netbits)
> +    {
> +        *netbits = bits;
> +    }
> +
> +    /* restore '/' separator, if any */
> +    if (sep)
> +    {
> +        *sep = '/';
> +    }
>

This restore is redundant as we are now working on a copy which
is going to be freed two lines below.


> +out:
> +    free(var_host);
> +
> +    return ret;
> +}
>

...

+/**
> + * Translate an IPv4 addr or hostname from string form to in_addr_t
> + *
> + * In case of resolve error, it will try again for
> + * resolve_retry_seconds seconds.
> + */
>

More docs is nice, but this is confusing. It will retry for
(resolve_retry_seconds / 5)
times will be more correct. Its as if someone thought if the caller says
try for 15
seconds its ok to try three times and wait 5 seconds in between. In reality
that
could take 90 seconds or even more in some cases as getaddrinfo may not
return
for 30 seconds or more. But that's a separate issue.

And, it will try only once if flags contain GETADDR_TRY_ONCE

 in_addr_t getaddr(unsigned int flags,
>                    const char *hostname,
>                    int resolve_retry_seconds,
>                    bool *succeeded,
>                    volatile int *signal_received);


The rest looks good though I've not tested it.

Selva
<div dir="ltr">Hi,<div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Dec 2, 2017 at 3:54 AM, Antonio Quartulli <span dir="ltr">&lt;<a href="mailto:a@unstable.cc" target="_blank">a@unstable.cc</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Similarly to ifconfig(-push), its IPv6 counterpart is now able to<br>
accept hostnames as well instead of IP addresses in numeric form.<br></blockquote><div><br></div><div>If dns names currently work for ifconfig-push (I didn&#39;t know),  makes sense to</div><div>support it for ipv6 as well.</div><div><br></div><div>But getaddrinfo can take a long time to timeout, so we are adding another</div><div>potentially blocking call on the server. Should not be an issue on a well</div><div>administered server, but just saying...</div><div><br></div><div>Otherwise looks good except for a couple of issues as below:</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Basically this means that the user is now allowed to specify<br>
something like this:<br>
<br>
ifconfig-ipv6-push <a href="http://my.hostname.cx/64" rel="noreferrer" target="_blank">my.hostname.cx/64</a><br>
<br>
This is exactly the same behaviour that we already have with<br>
ifconfig(-push).<br>
<br>
The generic code introduced in this patch will be later used to<br>
implement the /bits parsing support for IPv4 addresses.<br>
<br>
Trac: #808<br>
Signed-off-by: Antonio Quartulli &lt;a@unstable.cc&gt;<br>
---<br>
<br>
v2:<br>
- rebased on top of master<br>
- style adapted to new CodingStyle<br>
<br>
 src/openvpn/options.c |  61 ------------------------<br>
 src/openvpn/options.h |   4 --<br>
 src/openvpn/socket.c  | 126 ++++++++++++++++++++++++++++++<wbr>+++++++++++++++-----<br>
 src/openvpn/socket.h  |  12 +++++<br>
 4 files changed, 126 insertions(+), 77 deletions(-)<br>
<br>
diff --git a/src/openvpn/options.c b/src/openvpn/options.c<br>
index 8e5cdf7f..767cdaeb 100644<br>
--- a/src/openvpn/options.c<br>
+++ b/src/openvpn/options.c<br>
@@ -1033,67 +1033,6 @@ get_ip_addr(const char *ip_string, int msglevel, bool *error)<br>
     return ret;<br>
 }<br>
<br>
-/* helper: parse a text string containing an IPv6 address + netbits<br>
- * in &quot;standard format&quot; (2001:dba::/32)<br>
- * &quot;/nn&quot; is optional, default to /64 if missing<br>
- *<br>
- * return true if parsing succeeded, modify *network and *netbits<br>
- */<br>
-bool<br>
-get_ipv6_addr( const char *prefix_str, struct in6_addr *network,<br>
-               unsigned int *netbits, int msglevel)<br>
-{<br>
-    char *sep, *endp;<br>
-    int bits;<br>
-    struct in6_addr t_network;<br>
-<br>
-    sep = strchr( prefix_str, &#39;/&#39; );<br>
-    if (sep == NULL)<br>
-    {<br>
-        bits = 64;<br>
-    }<br>
-    else<br>
-    {<br>
-        bits = strtol( sep+1, &amp;endp, 10 );<br>
-        if (*endp != &#39;\0&#39; || bits &lt; 0 || bits &gt; 128)<br>
-        {<br>
-            msg(msglevel, &quot;IPv6 prefix &#39;%s&#39;: invalid &#39;/bits&#39; spec&quot;, prefix_str);<br>
-            return false;<br>
-        }<br>
-    }<br>
-<br>
-    /* temporary replace &#39;/&#39; in caller-provided string with &#39;\0&#39;, otherwise<br>
-     * inet_pton() will refuse prefix string<br>
-     * (alternative would be to strncpy() the prefix to temporary buffer)<br>
-     */<br>
-<br>
-    if (sep != NULL)<br>
-    {<br>
-        *sep = &#39;\0&#39;;<br>
-    }<br>
-<br>
-    if (inet_pton( AF_INET6, prefix_str, &amp;t_network ) != 1)<br>
-    {<br>
-        msg(msglevel, &quot;IPv6 prefix &#39;%s&#39;: invalid IPv6 address&quot;, prefix_str);<br>
-        return false;<br>
-    }<br>
-<br>
-    if (sep != NULL)<br>
-    {<br>
-        *sep = &#39;/&#39;;<br>
-    }<br>
-<br>
-    if (netbits != NULL)<br>
-    {<br>
-        *netbits = bits;<br>
-    }<br>
-    if (network != NULL)<br>
-    {<br>
-        *network = t_network;<br>
-    }<br>
-    return true;                /* parsing OK, values set */<br>
-}<br>
-<br>
 /**<br>
  * Returns newly allocated string containing address part without &quot;/nn&quot;.<br>
  *<br>
diff --git a/src/openvpn/options.h b/src/openvpn/options.h<br>
index 035c6d15..d67c2785 100644<br>
--- a/src/openvpn/options.h<br>
+++ b/src/openvpn/options.h<br>
@@ -817,8 +817,4 @@ void options_string_import(struct options *options,<br>
                            unsigned int *option_types_found,<br>
                            struct env_set *es);<br>
<br>
-bool get_ipv6_addr( const char *prefix_str, struct in6_addr *network,<br>
-                    unsigned int *netbits, int msglevel );<br>
-<br>
-<br>
 #endif /* ifndef OPTIONS_H */<br>
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c<br>
index 0fc91f21..4cadae23 100644<br>
--- a/src/openvpn/socket.c<br>
+++ b/src/openvpn/socket.c<br>
@@ -74,12 +74,102 @@ sf2gaf(const unsigned int getaddr_flags,<br>
 /*<br>
  * Functions related to the translation of DNS names to IP addresses.<br>
  */<br>
+static int<br>
+get_addr_generic(sa_family_t af, unsigned int flags, const char *hostname,<br>
+                 void *network, unsigned int *netbits,<br>
+                 int resolve_retry_seconds, volatile int *signal_received,<br>
+                 int msglevel)<br>
+{<br>
+    char *endp, *sep, *var_host;<br>
+    uint8_t bits, max_bits;<br>
+    struct addrinfo *ai;<br>
+    int ret = -1;<br>
+<br>
+    ASSERT(hostname);<br>
+<br>
+    /* assign family specific default values */<br>
+    switch (af)<br>
+    {<br>
+        case AF_INET:<br>
+            bits = 0;<br>
+            max_bits = sizeof(in_addr_t) * 8;<br>
+            break;<br>
+        case AF_INET6:<br>
+            bits = 64;<br>
+            max_bits = sizeof(struct in6_addr) * 8;<br>
+            break;<br>
+        default:<br>
+            ASSERT(0);<br>
+    }<br>
+<br>
+    /* we need to modify the hostname received as input, but we don&#39;t want to<br>
+     * touch it directly as it might be a constant string.<br>
+     *<br>
+     * Therefore, we clone the string here and free it at the end of the<br>
+     * function */<br>
+    var_host = strdup(hostname);<br>
+    ASSERT(var_host);<br></blockquote><div><br></div><div>I think ASSERT should be used only to catch errors in coding logic,</div><div>not for plausible runtime errors like this. Especially since this happens</div><div>on the server, no reason to terminate the process here.</div><div><br></div><div>Instead, log an error (M_NONFTAL|M_ERRNO will do nicely) and return -1</div><div>or goto out. The option parser will log a generic warning, but still useful </div><div>to log here using M_ERRNO for more specific info.</div><div><br></div><div>Alternatively one could use a buffer on the stack --  easy to do as good </div><div>old dns names are limited in size (255 octets max?) and the current option</div><div>parser also limits the argument passed here to &lt; 255 bytes. But if we support</div><div>internationalized domain names (currently we do not, do we?) and if the line</div><div>length in option parser is ever increased, a much larger buffer would be needed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+    /* check if this hostname has a /bits suffix */<br>
+    sep = strchr(var_host , &#39;/&#39;);<br>
+    if (sep)<br>
+    {<br>
+        bits = strtoul(sep + 1, &amp;endp, 10);</blockquote><div><br></div><div>There are a number of such type coercions in the patch </div><div>(ulong to uint8_t, size_t  to unit8_t etc.) that some compilers (aka MSVC:)</div><div>may warn about, but I do not personally care. All are safe and deliberate</div><div>except for the nitpicking below.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+        if ((*endp != &#39;\0&#39;) || (bits &gt; max_bits))</blockquote><div><br></div><div>That (bits &gt; max_bits) check will not catch many input errors, as the</div><div>input is  already truncated to uint8_t. For example, /255 will be flagged</div><div>as an error, but /256 will pass as 0.</div><div><br></div><div>Though its not possible to catch all input errors, keeping bits as unsigned</div><div>int (instead of uint8_t) may be better here. That&#39;ll  also match netbits</div><div>in the function signature, return value of strtoul etc..</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+        {<br>
+            msg(msglevel, &quot;IP prefix &#39;%s&#39;: invalid &#39;/bits&#39; spec&quot;, hostname);<br>
+            goto out;<br>
+        }<br>
+        /* temporary truncate string at &#39;/&#39;. This allows the IP<br>
+         * parsing routines to properly work. Will be restored later.<br>
+         */<br></blockquote><div><br></div><div>This comment is no longer required as there is no need to restore &#39;/&#39;</div><div>(also see below)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+        *sep = &#39;\0&#39;;<br>
+    }<br>
+<br>
+    ret = openvpn_getaddrinfo(flags &amp; ~GETADDR_HOST_ORDER, var_host, NULL,<br>
+                              resolve_retry_seconds, signal_received, af, &amp;ai);<br>
+    if ((ret == 0) &amp;&amp; network)<br>
+    {<br>
+        struct in6_addr *ip6;<br>
+        in_addr_t *ip4;<br>
+<br>
+        switch (af)<br>
+        {<br>
+            case AF_INET:<br>
+                ip4 = network;<br>
+                *ip4 = ((struct sockaddr_in *)ai-&gt;ai_addr)-&gt;sin_addr.s_<wbr>addr;<br>
+<br>
+                if (flags &amp; GETADDR_HOST_ORDER)<br>
+                {<br>
+                    *ip4 = ntohl(*ip4);<br>
+                }<br>
+                break;<br>
+            case AF_INET6:<br>
+                ip6 = network;<br>
+                *ip6 = ((struct sockaddr_in6 *)ai-&gt;ai_addr)-&gt;sin6_addr;<br>
+                break;<br>
+            default:<br>
+                ASSERT(0);<br>
+        }<br>
+        freeaddrinfo(ai);<br>
+    }<br>
+<br>
+    if (netbits)<br>
+    {<br>
+        *netbits = bits;<br>
+    }<br>
+<br>
+    /* restore &#39;/&#39; separator, if any */<br>
+    if (sep)<br>
+    {<br>
+        *sep = &#39;/&#39;;<br>
+    }<br>
</blockquote><div><br></div><div>This restore is redundant as we are now working on a copy which</div><div>is going to be freed two lines below.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+out:<br>
+    free(var_host);<br>
+<br>
+    return ret;<br>
+}<br></blockquote><div><br></div><div>...</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="font-size:12.8px">+/**<br></span><span style="font-size:12.8px">+ * Translate an IPv4 addr or hostname from string form to in_addr_t<br></span><span style="font-size:12.8px">+ *<br></span><span style="font-size:12.8px">+ * In case of resolve error, it will try again for<br></span><span style="font-size:12.8px">+ * resolve_retry_seconds seconds.<br></span><span style="font-size:12.8px">+ */<br></span></blockquote><div><br></div><div>More docs is nice, but this is confusing. It will retry for (resolve_retry_seconds / 5)</div><div>times will be more correct. Its as if someone thought if the caller says try for 15</div><div>seconds its ok to try three times and wait 5 seconds in between. In reality that</div><div>could take 90 seconds or even more in some cases as getaddrinfo may not return</div><div>for 30 seconds or more. But that&#39;s a separate issue.</div><div><br></div><div>And, it will try only once if flags contain GETADDR_TRY_ONCE<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="font-size:12.8px"></span><span style="font-size:12.8px"> in_addr_t getaddr(unsigned int flags,<br></span><span style="font-size:12.8px">                   const char *hostname,<br></span><span style="font-size:12.8px">                   int resolve_retry_seconds,<br></span><span style="font-size:12.8px">                   bool *succeeded,<br></span><span style="font-size:12.8px">                   volatile int *signal_received);</span></blockquote><div> </div><div>The rest looks good though I&#39;ve not tested it.</div><div><br></div><div>Selva</div></div></div></div></div>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Antonio Quartulli Dec. 3, 2017, 2:25 a.m. | #2
Hi,

On 03/12/17 04:27, Selva Nair wrote:
> Hi,
> 
> On Sat, Dec 2, 2017 at 3:54 AM, Antonio Quartulli <a@unstable.cc> wrote:
> 
>> Similarly to ifconfig(-push), its IPv6 counterpart is now able to
>> accept hostnames as well instead of IP addresses in numeric form.
>>
> 
> If dns names currently work for ifconfig-push (I didn't know),  makes sense
> to
> support it for ipv6 as well.
> 
> But getaddrinfo can take a long time to timeout, so we are adding another
> potentially blocking call on the server. Should not be an issue on a well
> administered server, but just saying...

Yeah, I think this the 'drawback' that the admin has to consider if you
want hostnames in the config.


> 
> Otherwise looks good except for a couple of issues as below:
> 
> 
>> Basically this means that the user is now allowed to specify
>> something like this:
>>
>> ifconfig-ipv6-push my.hostname.cx/64
>>
>> This is exactly the same behaviour that we already have with
>> ifconfig(-push).
>>
>> The generic code introduced in this patch will be later used to
>> implement the /bits parsing support for IPv4 addresses.
>>
>> Trac: #808
>> Signed-off-by: Antonio Quartulli <a@unstable.cc>
>> ---
>>
>> v2:
>> - rebased on top of master
>> - style adapted to new CodingStyle
>>
>>  src/openvpn/options.c |  61 ------------------------
>>  src/openvpn/options.h |   4 --
>>  src/openvpn/socket.c  | 126 ++++++++++++++++++++++++++++++
>> +++++++++++++++-----
>>  src/openvpn/socket.h  |  12 +++++
>>  4 files changed, 126 insertions(+), 77 deletions(-)
>>
>> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
>> index 8e5cdf7f..767cdaeb 100644
>> --- a/src/openvpn/options.c
>> +++ b/src/openvpn/options.c
>> @@ -1033,67 +1033,6 @@ get_ip_addr(const char *ip_string, int msglevel,
>> bool *error)
>>      return ret;
>>  }
>>
>> -/* helper: parse a text string containing an IPv6 address + netbits
>> - * in "standard format" (2001:dba::/32)
>> - * "/nn" is optional, default to /64 if missing
>> - *
>> - * return true if parsing succeeded, modify *network and *netbits
>> - */
>> -bool
>> -get_ipv6_addr( const char *prefix_str, struct in6_addr *network,
>> -               unsigned int *netbits, int msglevel)
>> -{
>> -    char *sep, *endp;
>> -    int bits;
>> -    struct in6_addr t_network;
>> -
>> -    sep = strchr( prefix_str, '/' );
>> -    if (sep == NULL)
>> -    {
>> -        bits = 64;
>> -    }
>> -    else
>> -    {
>> -        bits = strtol( sep+1, &endp, 10 );
>> -        if (*endp != '\0' || bits < 0 || bits > 128)
>> -        {
>> -            msg(msglevel, "IPv6 prefix '%s': invalid '/bits' spec",
>> prefix_str);
>> -            return false;
>> -        }
>> -    }
>> -
>> -    /* temporary replace '/' in caller-provided string with '\0',
>> otherwise
>> -     * inet_pton() will refuse prefix string
>> -     * (alternative would be to strncpy() the prefix to temporary buffer)
>> -     */
>> -
>> -    if (sep != NULL)
>> -    {
>> -        *sep = '\0';
>> -    }
>> -
>> -    if (inet_pton( AF_INET6, prefix_str, &t_network ) != 1)
>> -    {
>> -        msg(msglevel, "IPv6 prefix '%s': invalid IPv6 address",
>> prefix_str);
>> -        return false;
>> -    }
>> -
>> -    if (sep != NULL)
>> -    {
>> -        *sep = '/';
>> -    }
>> -
>> -    if (netbits != NULL)
>> -    {
>> -        *netbits = bits;
>> -    }
>> -    if (network != NULL)
>> -    {
>> -        *network = t_network;
>> -    }
>> -    return true;                /* parsing OK, values set */
>> -}
>> -
>>  /**
>>   * Returns newly allocated string containing address part without "/nn".
>>   *
>> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
>> index 035c6d15..d67c2785 100644
>> --- a/src/openvpn/options.h
>> +++ b/src/openvpn/options.h
>> @@ -817,8 +817,4 @@ void options_string_import(struct options *options,
>>                             unsigned int *option_types_found,
>>                             struct env_set *es);
>>
>> -bool get_ipv6_addr( const char *prefix_str, struct in6_addr *network,
>> -                    unsigned int *netbits, int msglevel );
>> -
>> -
>>  #endif /* ifndef OPTIONS_H */
>> diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
>> index 0fc91f21..4cadae23 100644
>> --- a/src/openvpn/socket.c
>> +++ b/src/openvpn/socket.c
>> @@ -74,12 +74,102 @@ sf2gaf(const unsigned int getaddr_flags,
>>  /*
>>   * Functions related to the translation of DNS names to IP addresses.
>>   */
>> +static int
>> +get_addr_generic(sa_family_t af, unsigned int flags, const char *hostname,
>> +                 void *network, unsigned int *netbits,
>> +                 int resolve_retry_seconds, volatile int *signal_received,
>> +                 int msglevel)
>> +{
>> +    char *endp, *sep, *var_host;
>> +    uint8_t bits, max_bits;
>> +    struct addrinfo *ai;
>> +    int ret = -1;
>> +
>> +    ASSERT(hostname);
>> +
>> +    /* assign family specific default values */
>> +    switch (af)
>> +    {
>> +        case AF_INET:
>> +            bits = 0;
>> +            max_bits = sizeof(in_addr_t) * 8;
>> +            break;
>> +        case AF_INET6:
>> +            bits = 64;
>> +            max_bits = sizeof(struct in6_addr) * 8;
>> +            break;
>> +        default:
>> +            ASSERT(0);
>> +    }
>> +
>> +    /* we need to modify the hostname received as input, but we don't
>> want to
>> +     * touch it directly as it might be a constant string.
>> +     *
>> +     * Therefore, we clone the string here and free it at the end of the
>> +     * function */
>> +    var_host = strdup(hostname);
>> +    ASSERT(var_host);
>>
> 
> I think ASSERT should be used only to catch errors in coding logic,
> not for plausible runtime errors like this. Especially since this happens
> on the server, no reason to terminate the process here.
> 
> Instead, log an error (M_NONFTAL|M_ERRNO will do nicely) and return -1
> or goto out. The option parser will log a generic warning, but still useful
> to log here using M_ERRNO for more specific info.
> 

I agree. I am also not a big fan of ASSERT(). I think I had copy/pasted
this chunk of code from somewhere else (and consider that this patch is
quite old even though I have re-sent it now).

I will get rid of the ASSERTs.

> Alternatively one could use a buffer on the stack --  easy to do as good
> old dns names are limited in size (255 octets max?) and the current option
> parser also limits the argument passed here to < 255 bytes. But if we
> support
> internationalized domain names (currently we do not, do we?) and if the line
> length in option parser is ever increased, a much larger buffer would be
> needed.

Are you sure we have a limit of 255 octects? I am not so sure. Anyway,
this is not extremely important for now.

> 
> 
>> +
>> +    /* check if this hostname has a /bits suffix */
>> +    sep = strchr(var_host , '/');
>> +    if (sep)
>> +    {
>> +        bits = strtoul(sep + 1, &endp, 10);
> 
> 
> There are a number of such type coercions in the patch
> (ulong to uint8_t, size_t  to unit8_t etc.) that some compilers (aka MSVC:)
> may warn about, but I do not personally care. All are safe and deliberate
> except for the nitpicking below.
> 
> +        if ((*endp != '\0') || (bits > max_bits))
> 
> 
> That (bits > max_bits) check will not catch many input errors, as the
> input is  already truncated to uint8_t. For example, /255 will be flagged
> as an error, but /256 will pass as 0.
> 

These are probably all copy/paste from other parts of the code. So the
logic here is also used in other parts of the code. If we believe this
is not the proper way to handle these cases, I'd suggest to take a note
and fix these behaviors with a dedicated patch.

> Though its not possible to catch all input errors, keeping bits as unsigned
> int (instead of uint8_t) may be better here. That'll  also match netbits
> in the function signature, return value of strtoul etc..
> 
> +        {
>> +            msg(msglevel, "IP prefix '%s': invalid '/bits' spec",
>> hostname);
>> +            goto out;
>> +        }
>> +        /* temporary truncate string at '/'. This allows the IP
>> +         * parsing routines to properly work. Will be restored later.
>> +         */
>>
> 
> This comment is no longer required as there is no need to restore '/'
> (also see below)
> 
> 
>> +        *sep = '\0';
>> +    }
>> +
>> +    ret = openvpn_getaddrinfo(flags & ~GETADDR_HOST_ORDER, var_host, NULL,
>> +                              resolve_retry_seconds, signal_received, af,
>> &ai);
>> +    if ((ret == 0) && network)
>> +    {
>> +        struct in6_addr *ip6;
>> +        in_addr_t *ip4;
>> +
>> +        switch (af)
>> +        {
>> +            case AF_INET:
>> +                ip4 = network;
>> +                *ip4 = ((struct sockaddr_in *)ai->ai_addr)->sin_addr.s_
>> addr;
>> +
>> +                if (flags & GETADDR_HOST_ORDER)
>> +                {
>> +                    *ip4 = ntohl(*ip4);
>> +                }
>> +                break;
>> +            case AF_INET6:
>> +                ip6 = network;
>> +                *ip6 = ((struct sockaddr_in6 *)ai->ai_addr)->sin6_addr;
>> +                break;
>> +            default:
>> +                ASSERT(0);
>> +        }
>> +        freeaddrinfo(ai);
>> +    }
>> +
>> +    if (netbits)
>> +    {
>> +        *netbits = bits;
>> +    }
>> +
>> +    /* restore '/' separator, if any */
>> +    if (sep)
>> +    {
>> +        *sep = '/';
>> +    }
>>
> 
> This restore is redundant as we are now working on a copy which
> is going to be freed two lines below.

Absolutely right. I thought I had removed this though....thanks for
catching it!


> 
> 
>> +out:
>> +    free(var_host);
>> +
>> +    return ret;
>> +}
>>
> 
> ...
> 
> +/**
>> + * Translate an IPv4 addr or hostname from string form to in_addr_t
>> + *
>> + * In case of resolve error, it will try again for
>> + * resolve_retry_seconds seconds.
>> + */
>>
> 
> More docs is nice, but this is confusing. It will retry for
> (resolve_retry_seconds / 5)
> times will be more correct. Its as if someone thought if the caller says
> try for 15
> seconds its ok to try three times and wait 5 seconds in between. In reality
> that
> could take 90 seconds or even more in some cases as getaddrinfo may not
> return
> for 30 seconds or more. But that's a separate issue.
> 
> And, it will try only once if flags contain GETADDR_TRY_ONCE
> 
>  in_addr_t getaddr(unsigned int flags,
>>                    const char *hostname,
>>                    int resolve_retry_seconds,
>>                    bool *succeeded,
>>                    volatile int *signal_received);
> 
> 
> The rest looks good though I've not tested it.

Thanks a lot for your review!

I'll address the issues you raised and send v3.

Cheers,
Selva Nair Dec. 3, 2017, 3:39 a.m. | #3
oops forgot to cc the list..

---------- Forwarded message ----------
From: Selva Nair <selva.nair@gmail.com>
Date: Sat, Dec 2, 2017 at 10:16 PM
Subject: Re: [Openvpn-devel] [PATCH v2] ifconfig-ipv6(-push): allow using
hostnames
To: Antonio Quartulli <a@unstable.cc>


Hi,

On Sat, Dec 2, 2017 at 9:25 PM, Antonio Quartulli <a@unstable.cc> wrote:

> Hi,
>
> On 03/12/17 04:27, Selva Nair wrote:
>
> >> +    /* we need to modify the hostname received as input, but we don't
> >> want to
> >> +     * touch it directly as it might be a constant string.
> >> +     *
> >> +     * Therefore, we clone the string here and free it at the end of
> the
> >> +     * function */
> >> +    var_host = strdup(hostname);
> >> +    ASSERT(var_host);
> >>
> >
> > I think ASSERT should be used only to catch errors in coding logic,
> > not for plausible runtime errors like this. Especially since this happens
> > on the server, no reason to terminate the process here.
> >
> > Instead, log an error (M_NONFTAL|M_ERRNO will do nicely) and return -1
> > or goto out. The option parser will log a generic warning, but still
> useful
> > to log here using M_ERRNO for more specific info.
> >
>
> I agree. I am also not a big fan of ASSERT(). I think I had copy/pasted
> this chunk of code from somewhere else (and consider that this patch is
> quite old even though I have re-sent it now).
>
> I will get rid of the ASSERTs.
>
> > Alternatively one could use a buffer on the stack --  easy to do as good
> > old dns names are limited in size (255 octets max?) and the current
> option
> > parser also limits the argument passed here to < 255 bytes. But if we
> > support
> > internationalized domain names (currently we do not, do we?) and if the
> line
> > length in option parser is ever increased, a much larger buffer would be
> > needed.
>
> Are you sure we have a limit of 255 octects? I am not so sure. Anyway,
> this is not extremely important for now.
>

Yes, not important for this patch and stdup is anyway fine as long as we do
not
ASSERT.

The 255 byte limit comes from the config file or option parser which reads
the
input from ccd or client-connect script output. The length of a line is
limited
to  #define OPTION_LINE_SIZE 256 in options.h


> >
> >
> >> +
> >> +    /* check if this hostname has a /bits suffix */
> >> +    sep = strchr(var_host , '/');
> >> +    if (sep)
> >> +    {
> >> +        bits = strtoul(sep + 1, &endp, 10);
> >
> >
> > There are a number of such type coercions in the patch
> > (ulong to uint8_t, size_t  to unit8_t etc.) that some compilers (aka
> MSVC:)
> > may warn about, but I do not personally care. All are safe and deliberate
> > except for the nitpicking below.
> >
> > +        if ((*endp != '\0') || (bits > max_bits))
> >
> >
> > That (bits > max_bits) check will not catch many input errors, as the
> > input is  already truncated to uint8_t. For example, /255 will be flagged
> > as an error, but /256 will pass as 0.
> >
>
> These are probably all copy/paste from other parts of the code. So the
> logic here is also used in other parts of the code. If we believe this
> is not the proper way to handle these cases, I'd suggest to take a note
> and fix these behaviors with a dedicated patch.
>

Please bear with me: Quoting from the patch:

-    char *sep, *endp;
-    int bits;
-    struct in6_addr t_network;

So, the original code had bits defined as int, so why change to unit8_t?
Changing the original int to unsigned int would've made sense
as we are parsing using strtoul and returning the result in an unsigned int.
But uint8_t brings in hardly any gain and causes all those regressions and
type conversions I pointed out..

> Though its not possible to catch all input errors, keeping bits as
> unsigned
> > int (instead of uint8_t) may be better here. That'll  also match netbits
> > in the function signature, return value of strtoul etc..


Regards,

Selva
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Antonio Quartulli Dec. 3, 2017, 3:46 a.m. | #4
Hi,

On 03/12/17 11:39, Selva Nair wrote:
> oops forgot to cc the list..
> 
> ---------- Forwarded message ----------
> From: Selva Nair <selva.nair@gmail.com>
> Date: Sat, Dec 2, 2017 at 10:16 PM
> Subject: Re: [Openvpn-devel] [PATCH v2] ifconfig-ipv6(-push): allow using
> hostnames
> To: Antonio Quartulli <a@unstable.cc>
> 
> 
> Hi,
> 
> On Sat, Dec 2, 2017 at 9:25 PM, Antonio Quartulli <a@unstable.cc> wrote:
> 
>> Hi,
>>
>> On 03/12/17 04:27, Selva Nair wrote:
>>
>>>> +    /* we need to modify the hostname received as input, but we don't
>>>> want to
>>>> +     * touch it directly as it might be a constant string.
>>>> +     *
>>>> +     * Therefore, we clone the string here and free it at the end of
>> the
>>>> +     * function */
>>>> +    var_host = strdup(hostname);
>>>> +    ASSERT(var_host);
>>>>
>>>
>>> I think ASSERT should be used only to catch errors in coding logic,
>>> not for plausible runtime errors like this. Especially since this happens
>>> on the server, no reason to terminate the process here.
>>>
>>> Instead, log an error (M_NONFTAL|M_ERRNO will do nicely) and return -1
>>> or goto out. The option parser will log a generic warning, but still
>> useful
>>> to log here using M_ERRNO for more specific info.
>>>
>>
>> I agree. I am also not a big fan of ASSERT(). I think I had copy/pasted
>> this chunk of code from somewhere else (and consider that this patch is
>> quite old even though I have re-sent it now).
>>
>> I will get rid of the ASSERTs.
>>
>>> Alternatively one could use a buffer on the stack --  easy to do as good
>>> old dns names are limited in size (255 octets max?) and the current
>> option
>>> parser also limits the argument passed here to < 255 bytes. But if we
>>> support
>>> internationalized domain names (currently we do not, do we?) and if the
>> line
>>> length in option parser is ever increased, a much larger buffer would be
>>> needed.
>>
>> Are you sure we have a limit of 255 octects? I am not so sure. Anyway,
>> this is not extremely important for now.
>>
> 
> Yes, not important for this patch and stdup is anyway fine as long as we do
> not
> ASSERT.
> 
> The 255 byte limit comes from the config file or option parser which reads
> the
> input from ccd or client-connect script output. The length of a line is
> limited
> to  #define OPTION_LINE_SIZE 256 in options.h

oh ok. thanks for showing this.


> 
> 
>>>
>>>
>>>> +
>>>> +    /* check if this hostname has a /bits suffix */
>>>> +    sep = strchr(var_host , '/');
>>>> +    if (sep)
>>>> +    {
>>>> +        bits = strtoul(sep + 1, &endp, 10);
>>>
>>>
>>> There are a number of such type coercions in the patch
>>> (ulong to uint8_t, size_t  to unit8_t etc.) that some compilers (aka
>> MSVC:)
>>> may warn about, but I do not personally care. All are safe and deliberate
>>> except for the nitpicking below.
>>>
>>> +        if ((*endp != '\0') || (bits > max_bits))
>>>
>>>
>>> That (bits > max_bits) check will not catch many input errors, as the
>>> input is  already truncated to uint8_t. For example, /255 will be flagged
>>> as an error, but /256 will pass as 0.
>>>
>>
>> These are probably all copy/paste from other parts of the code. So the
>> logic here is also used in other parts of the code. If we believe this
>> is not the proper way to handle these cases, I'd suggest to take a note
>> and fix these behaviors with a dedicated patch.
>>
> 
> Please bear with me: Quoting from the patch:
> 
> -    char *sep, *endp;
> -    int bits;
> -    struct in6_addr t_network;
> 
> So, the original code had bits defined as int, so why change to unit8_t?
> Changing the original int to unsigned int would've made sense
> as we are parsing using strtoul and returning the result in an unsigned int.
> But uint8_t brings in hardly any gain and causes all those regressions and
> type conversions I pointed out..

right. I am converting bits to unsigned long (which is what is returned
by strtoul()) and left max_bits as uint8_t.


Cheers,
Selva Nair Dec. 3, 2017, 4:38 a.m. | #5
Hi,

Responding to this old version just to be on record.

I realized patch this was assigned to Gert on patchwork too late after
started responding on my own. Sorry for jumping the gun. Have to make
keeping an eye on patchwork a habit..

I'll leave the latest v4 alone.

cheers,

Selva
<div dir="ltr">Hi,<div><br></div><div>Responding to this old version just to be on record.</div><div><br></div><div>I realized patch this was assigned to Gert on patchwork too late after</div><div>started responding on my own. Sorry for jumping the gun. Have to make</div><div>keeping an eye on patchwork a habit..</div><div><br></div><div>I&#39;ll leave the latest v4 alone.</div><div><br></div><div>cheers,</div><div><br></div><div>Selva</div><div class="gmail_extra"><br></div></div>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Antonio Quartulli Dec. 3, 2017, 4:42 a.m. | #6
Hi,

On 03/12/17 12:38, Selva Nair wrote:
> Hi,
> 
> Responding to this old version just to be on record.
> 
> I realized patch this was assigned to Gert on patchwork too late after
> started responding on my own. Sorry for jumping the gun. Have to make
> keeping an eye on patchwork a habit..

No problem at all!
Actually it was not "assigned" to Gert, but *I* simply delegated it to
him, so that he could remember in the future (whenever he will have
time) that *I* though this patch would fall into his area of interest.

> 
> I'll leave the latest v4 alone.

As I said above there is no "assignment", just treat that as "reminders"
for other people. I am glad you jumped in and provided some early
feedback (and I am sure Gert is happy too).

So, please, feel free to comment further.


Cheers,
Gert Doering Dec. 3, 2017, 6:54 p.m. | #7
Hi,,

On Sat, Dec 02, 2017 at 11:38:28PM -0500, Selva Nair wrote:
> Responding to this old version just to be on record.
> 
> I realized patch this was assigned to Gert on patchwork too late after
> started responding on my own. Sorry for jumping the gun. Have to make
> keeping an eye on patchwork a habit..
> 
> I'll leave the latest v4 alone.

If that is the consequence of assigning patches to people on patchwork,
we should stop doing so (Antonio, are you listening? :-) ).  Obviously 
the patch was in dire need of a good review, and if you have time while 
I am busy with family things, this is more than welcome.

Voluntarily *taking* a patch for review is useful as a flag "I'm going to
handle this", but third-party-tagging "this is for Gert!" seems to have
potential for backfiring, so maybe we shouldn't do that, then.

(There currently is one patch in patchwork which I took myself, and that's 
the "lowest metric interface" one)

gert
Selva Nair Dec. 3, 2017, 7:19 p.m. | #8
On Sun, Dec 3, 2017 at 1:54 PM, Gert Doering <gert@greenie.muc.de> wrote:

> Hi,,
>
> On Sat, Dec 02, 2017 at 11:38:28PM -0500, Selva Nair wrote:
> > Responding to this old version just to be on record.
> >
> > I realized patch this was assigned to Gert on patchwork too late after
> > started responding on my own. Sorry for jumping the gun. Have to make
> > keeping an eye on patchwork a habit..
> >
> > I'll leave the latest v4 alone.

If that is the consequence of assigning patches to people on patchwork,
> we should stop doing so (Antonio, are you listening? :-) ).


I didn't want to step on anyone's toes... But based on Antonio's
explanation,
I take that remark back. Will look into v4 as soon as I get some time to
test it.


> Obviously
> the patch was in dire need of a good review, and if you have time while
> I am busy with family things, this is more than welcome.
>
> Voluntarily *taking* a patch for review is useful as a flag "I'm going to
> handle this", but third-party-tagging "this is for Gert!" seems to have
> potential for backfiring, so maybe we shouldn't do that, then.


Agreed. I just didn't realize Antonio delegated it to you and assumed
all "delegation" are indications that the person has volunteered to look
into it. As for me, I totally welcome others reviewing something I delegated
to myself, or commenting/criticizing/extoling (or tipping :) my reviews
(multiple reviews are indeed great).

But I wouldn't particularly want others delegating work to me :)
So let's continue the practice of delegating to self, not to others.
We can always use the list (or IRC for those who frequent the channel)
for requesting reviews etc.

Best,

Selva
<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Dec 3, 2017 at 1:54 PM, Gert Doering <span dir="ltr">&lt;<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,,<br>
<span class=""><br>
On Sat, Dec 02, 2017 at 11:38:28PM -0500, Selva Nair wrote:<br>
&gt; Responding to this old version just to be on record.<br>
&gt;<br>
&gt; I realized patch this was assigned to Gert on patchwork too late after<br>
&gt; started responding on my own. Sorry for jumping the gun. Have to make<br>
&gt; keeping an eye on patchwork a habit..<br>
&gt;<br>
&gt; I&#39;ll leave the latest v4 alone.</span> </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">If that is the consequence of assigning patches to people on patchwork,<br>
we should stop doing so (Antonio, are you listening? :-) ).  </blockquote><div><br></div><div>I didn&#39;t want to step on anyone&#39;s toes... But based on Antonio&#39;s explanation,</div><div>I take that remark back. Will look into v4 as soon as I get some time to test it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Obviously<br>
the patch was in dire need of a good review, and if you have time while<br>
I am busy with family things, this is more than welcome.<br>
<br>
Voluntarily *taking* a patch for review is useful as a flag &quot;I&#39;m going to<br>
handle this&quot;, but third-party-tagging &quot;this is for Gert!&quot; seems to have<br>
potential for backfiring, so maybe we shouldn&#39;t do that, then.</blockquote><div><br></div><div>Agreed. I just didn&#39;t realize Antonio delegated it to you and assumed</div><div>all &quot;delegation&quot; are indications that the person has volunteered to look</div><div>into it. As for me, I totally welcome others reviewing something I delegated</div><div>to myself, or commenting/criticizing/extoling (or tipping :) my reviews </div><div>(multiple reviews are indeed great).</div><div><br></div><div>But I wouldn&#39;t particularly want others delegating work to me :) </div><div>So let&#39;s continue the practice of delegating to self, not to others.</div><div>We can always use the list (or IRC for those who frequent the channel)</div><div>for requesting reviews etc.</div><div><br></div><div>Best,</div><div><br></div><div>Selva</div></div></div></div>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 8e5cdf7f..767cdaeb 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1033,67 +1033,6 @@  get_ip_addr(const char *ip_string, int msglevel, bool *error)
     return ret;
 }
 
-/* helper: parse a text string containing an IPv6 address + netbits
- * in "standard format" (2001:dba::/32)
- * "/nn" is optional, default to /64 if missing
- *
- * return true if parsing succeeded, modify *network and *netbits
- */
-bool
-get_ipv6_addr( const char *prefix_str, struct in6_addr *network,
-               unsigned int *netbits, int msglevel)
-{
-    char *sep, *endp;
-    int bits;
-    struct in6_addr t_network;
-
-    sep = strchr( prefix_str, '/' );
-    if (sep == NULL)
-    {
-        bits = 64;
-    }
-    else
-    {
-        bits = strtol( sep+1, &endp, 10 );
-        if (*endp != '\0' || bits < 0 || bits > 128)
-        {
-            msg(msglevel, "IPv6 prefix '%s': invalid '/bits' spec", prefix_str);
-            return false;
-        }
-    }
-
-    /* temporary replace '/' in caller-provided string with '\0', otherwise
-     * inet_pton() will refuse prefix string
-     * (alternative would be to strncpy() the prefix to temporary buffer)
-     */
-
-    if (sep != NULL)
-    {
-        *sep = '\0';
-    }
-
-    if (inet_pton( AF_INET6, prefix_str, &t_network ) != 1)
-    {
-        msg(msglevel, "IPv6 prefix '%s': invalid IPv6 address", prefix_str);
-        return false;
-    }
-
-    if (sep != NULL)
-    {
-        *sep = '/';
-    }
-
-    if (netbits != NULL)
-    {
-        *netbits = bits;
-    }
-    if (network != NULL)
-    {
-        *network = t_network;
-    }
-    return true;                /* parsing OK, values set */
-}
-
 /**
  * Returns newly allocated string containing address part without "/nn".
  *
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 035c6d15..d67c2785 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -817,8 +817,4 @@  void options_string_import(struct options *options,
                            unsigned int *option_types_found,
                            struct env_set *es);
 
-bool get_ipv6_addr( const char *prefix_str, struct in6_addr *network,
-                    unsigned int *netbits, int msglevel );
-
-
 #endif /* ifndef OPTIONS_H */
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 0fc91f21..4cadae23 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -74,12 +74,102 @@  sf2gaf(const unsigned int getaddr_flags,
 /*
  * Functions related to the translation of DNS names to IP addresses.
  */
+static int
+get_addr_generic(sa_family_t af, unsigned int flags, const char *hostname,
+                 void *network, unsigned int *netbits,
+                 int resolve_retry_seconds, volatile int *signal_received,
+                 int msglevel)
+{
+    char *endp, *sep, *var_host;
+    uint8_t bits, max_bits;
+    struct addrinfo *ai;
+    int ret = -1;
+
+    ASSERT(hostname);
+
+    /* assign family specific default values */
+    switch (af)
+    {
+        case AF_INET:
+            bits = 0;
+            max_bits = sizeof(in_addr_t) * 8;
+            break;
+        case AF_INET6:
+            bits = 64;
+            max_bits = sizeof(struct in6_addr) * 8;
+            break;
+        default:
+            ASSERT(0);
+    }
+
+    /* we need to modify the hostname received as input, but we don't want to
+     * touch it directly as it might be a constant string.
+     *
+     * Therefore, we clone the string here and free it at the end of the
+     * function */
+    var_host = strdup(hostname);
+    ASSERT(var_host);
+
+    /* check if this hostname has a /bits suffix */
+    sep = strchr(var_host , '/');
+    if (sep)
+    {
+        bits = strtoul(sep + 1, &endp, 10);
+        if ((*endp != '\0') || (bits > max_bits))
+        {
+            msg(msglevel, "IP prefix '%s': invalid '/bits' spec", hostname);
+            goto out;
+        }
+        /* temporary truncate string at '/'. This allows the IP
+         * parsing routines to properly work. Will be restored later.
+         */
+        *sep = '\0';
+    }
+
+    ret = openvpn_getaddrinfo(flags & ~GETADDR_HOST_ORDER, var_host, NULL,
+                              resolve_retry_seconds, signal_received, af, &ai);
+    if ((ret == 0) && network)
+    {
+        struct in6_addr *ip6;
+        in_addr_t *ip4;
+
+        switch (af)
+        {
+            case AF_INET:
+                ip4 = network;
+                *ip4 = ((struct sockaddr_in *)ai->ai_addr)->sin_addr.s_addr;
+
+                if (flags & GETADDR_HOST_ORDER)
+                {
+                    *ip4 = ntohl(*ip4);
+                }
+                break;
+            case AF_INET6:
+                ip6 = network;
+                *ip6 = ((struct sockaddr_in6 *)ai->ai_addr)->sin6_addr;
+                break;
+            default:
+                ASSERT(0);
+        }
+        freeaddrinfo(ai);
+    }
+
+    if (netbits)
+    {
+        *netbits = bits;
+    }
+
+    /* restore '/' separator, if any */
+    if (sep)
+    {
+        *sep = '/';
+    }
+out:
+    free(var_host);
+
+    return ret;
+}
 
-/*
- * Translate IP addr or hostname to in_addr_t.
- * If resolve error, try again for
- * resolve_retry_seconds seconds.
- */
 in_addr_t
 getaddr(unsigned int flags,
         const char *hostname,
@@ -87,20 +177,19 @@  getaddr(unsigned int flags,
         bool *succeeded,
         volatile int *signal_received)
 {
-    struct addrinfo *ai;
+    in_addr_t addr;
     int status;
-    status = openvpn_getaddrinfo(flags & ~GETADDR_HOST_ORDER, hostname, NULL,
-                                 resolve_retry_seconds, signal_received, AF_INET, &ai);
+
+    status = get_addr_generic(AF_INET, flags, hostname, &addr, NULL,
+                              resolve_retry_seconds, signal_received,
+                              M_WARN);
     if (status==0)
     {
-        struct in_addr ia;
         if (succeeded)
         {
             *succeeded = true;
         }
-        ia = ((struct sockaddr_in *)ai->ai_addr)->sin_addr;
-        freeaddrinfo(ai);
-        return (flags & GETADDR_HOST_ORDER) ? ntohl(ia.s_addr) : ia.s_addr;
+        return addr;
     }
     else
     {
@@ -112,6 +201,19 @@  getaddr(unsigned int flags,
     }
 }
 
+bool
+get_ipv6_addr(const char *hostname, struct in6_addr *network,
+              unsigned int *netbits, int msglevel)
+{
+    if (get_addr_generic(AF_INET6, GETADDR_RESOLVE, hostname, network, netbits,
+                         0, NULL, msglevel) < 0)
+    {
+        return false;
+    }
+
+    return true;                /* parsing OK, values set */
+}
+
 static inline bool
 streqnull(const char *a, const char *b)
 {
diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h
index 2d7f2187..81e9e9ae 100644
--- a/src/openvpn/socket.h
+++ b/src/openvpn/socket.h
@@ -532,12 +532,24 @@  bool unix_socket_get_peer_uid_gid(const socket_descriptor_t sd, int *uid, int *g
 
 #define GETADDR_CACHE_MASK              (GETADDR_DATAGRAM|GETADDR_PASSIVE)
 
+/**
+ * Translate an IPv4 addr or hostname from string form to in_addr_t
+ *
+ * In case of resolve error, it will try again for
+ * resolve_retry_seconds seconds.
+ */
 in_addr_t getaddr(unsigned int flags,
                   const char *hostname,
                   int resolve_retry_seconds,
                   bool *succeeded,
                   volatile int *signal_received);
 
+/**
+ * Translate an IPv6 addr or hostname from string form to in6_addr
+ */
+bool get_ipv6_addr(const char *hostname, struct in6_addr *network,
+                   unsigned int *netbits, int msglevel);
+
 int openvpn_getaddrinfo(unsigned int flags,
                         const char *hostname,
                         const char *servname,