[Openvpn-devel,v3] Treat dhcp-option DNS6 and DNS identical

OpenVPN3 accepts both IPv4 and IPv6 with option-dhcp DNS but throws
an error for option-dhcp DNS6.

This patch makes OpenVPN2 accept IPv4/IPv6 for both DNS and DNS6

Patch V2: Put IPv6 parsing logic into own function similar as for for IPv4 DNS
---
 doc/openvpn.8         | 14 +++++---------
 src/openvpn/options.c | 39 ++++++++++++++++++++++++---------------
 2 files changed, 29 insertions(+), 24 deletions(-)

Hi,

On Wed, Jan 31, 2018 at 4:41 AM, Arne Schwabe <arne@rfc2549.org> wrote:
> OpenVPN3 accepts both IPv4 and IPv6 with option-dhcp DNS but throws
> an error for option-dhcp DNS6.
>
> This patch makes OpenVPN2 accept IPv4/IPv6 for both DNS and DNS6
>
> Patch V2: Put IPv6 parsing logic into own function similar as for for IPv4 DNS
> ---
>  doc/openvpn.8         | 14 +++++---------
>  src/openvpn/options.c | 39 ++++++++++++++++++++++++---------------
>  2 files changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/doc/openvpn.8 b/doc/openvpn.8
> index 43bbc217..d083b908 100644
> --- a/doc/openvpn.8
> +++ b/doc/openvpn.8
> @@ -5886,17 +5886,13 @@ across the VPN.
>  Set Connection\-specific DNS Suffix.
>
>  .B DNS addr \-\-
> -Set primary domain name server IPv4 address.  Repeat
> +Set primary domain name server IPv4 or IPv6 address.  Repeat
>  this option to set secondary DNS server addresses.
>
> -.B DNS6 addr \-\-
> -Set primary domain name server IPv6 address.  Repeat
> -this option to set secondary DNS server IPv6 addresses.
> -
> -Note: currently this is handled using netsh (the
> -existing DHCP code can only do IPv4 DHCP, and that protocol only
> -permits IPv4 addresses anywhere).  The option will be put into the
> -environment, so an
> +Note: DNS IPv6 servers are currently set using netsh (the existing
> +DHCP code can only do IPv4 DHCP, and that protocol only permits IPv4
> +addresses anywhere).  The option will be put into the environment, so
> +an
>  .B \-\-up
>  script could act upon it if needed.
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 75def7b6..f405d8a2 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -705,8 +705,7 @@ static const char usage_message[] =
>      "                    which allow multiple addresses,\n"
>      "                    --dhcp-option must be repeated.\n"
>      "                    DOMAIN name : Set DNS suffix\n"
> -    "                    DNS addr    : Set domain name server address(es) (IPv4)\n"
> -    "                    DNS6 addr   : Set domain name server address(es) (IPv6)\n"
> +    "                    DNS addr    : Set domain name server address(es) (IPv4 and IPv6)\n"
>      "                    NTP         : Set NTP server address(es)\n"
>      "                    NBDD        : Set NBDD server address(es)\n"
>      "                    WINS addr   : Set WINS server address(es)\n"
> @@ -1228,6 +1227,20 @@ show_tuntap_options(const struct tuntap_options *o)
>
>  #if defined(_WIN32) || defined(TARGET_ANDROID)
>  static void
> +dhcp_option_dns6_parse(const char *parm, struct in6_addr *dns6_list, int *len, int msglevel)
> +{
> +    struct in6_addr addr;
> +    if (*len >= N_DHCP_ADDR)
> +    {
> +        msg(msglevel, "--dhcp-option DNS: maximum of %d IPv6 dns servers can be specified",
> +            N_DHCP_ADDR);
> +    }
> +    else if (get_ipv6_addr(parm, &addr, NULL, msglevel))
> +    {
> +        dns6_list[(*len)++] = addr;
> +    }
> +}
> +static void
>  dhcp_option_address_parse(const char *name, const char *parm, in_addr_t *array, int *len, int msglevel)
>  {
>      if (*len >= N_DHCP_ADDR)
> @@ -7070,6 +7083,7 @@ add_option(struct options *options,
>      {
>          struct tuntap_options *o = &options->tuntap_options;
>          VERIFY_PERMISSION(OPT_P_IPWIN32);
> +        bool ipv6dns = false;
>
>          if (streq(p[1], "DOMAIN") && p[2])
>          {
> @@ -7090,22 +7104,17 @@ add_option(struct options *options,
>              }
>              o->netbios_node_type = t;
>          }
> -        else if (streq(p[1], "DNS") && p[2])
> +        else if ((streq(p[1], "DNS") || streq(p[1], "DNS6")) && p[2] && (!strstr(p[2], ":") || ipv6_addr_safe(p[2])))
>          {
> -            dhcp_option_address_parse("DNS", p[2], o->dns, &o->dns_len, msglevel);
> -        }
> -        else if (streq(p[1], "DNS6") && p[2] && ipv6_addr_safe(p[2]))
> -        {
> -            struct in6_addr addr;
> -            foreign_option(options, p, 3, es);
> -            if (o->dns6_len >= N_DHCP_ADDR)
> +            if (strstr(p[2], ":"))
>              {
> -                msg(msglevel, "--dhcp-option DNS6: maximum of %d dns servers can be specified",
> -                    N_DHCP_ADDR);
> +                ipv6dns=true;
> +                foreign_option(options, p, 3, es);
> +                dhcp_option_dns6_parse(p[2], o->dns6, &o->dns6_len, msglevel);
>              }
> -            else if (get_ipv6_addr(p[2], &addr, NULL, msglevel))
> +            else
>              {
> -                o->dns6[o->dns6_len++] = addr;
> +                dhcp_option_address_parse("DNS", p[2], o->dns, &o->dns_len, msglevel);
>              }
>          }
>          else if (streq(p[1], "WINS") && p[2])
> @@ -7133,7 +7142,7 @@ add_option(struct options *options,
>          /* flag that we have options to give to the TAP driver's DHCPv4 server
>           *  - skipped for "DNS6", as that's not a DHCPv4 option
>           */
> -        if (!streq(p[1], "DNS6"))
> +        if (!ipv6dns)
>          {
>              o->dhcp_options = true;
>          }

Acked-by: Selva Nair <selva.nair@gmail.com>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Your patch has been applied to the master and release/2.4 branch.

(2.4 because this is "long-term compatibility" stuff, and fairly 
non-intrusive)

I've taken the liberty to change the commit message and mention that it's
"v3" here :-)

commit 849006bf17bba524e6f3344598adcbe41bedf450 (master)
commit 61a72ecd59a57a50d0757b56b16f4ed3ce700f3f (release/2.4)
Author: Arne Schwabe
Date:   Wed Jan 31 10:41:02 2018 +0100

     Treat dhcp-option DNS6 and DNS identical

     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <1517391662-21325-1-git-send-email-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16413.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Hi,

On Sun, Feb 04, 2018 at 11:50:03AM +0100, Gert Doering wrote:
> I've taken the liberty to change the commit message and mention that it's
> "v3" here :-)
> 
> commit 849006bf17bba524e6f3344598adcbe41bedf450 (master)
> commit 61a72ecd59a57a50d0757b56b16f4ed3ce700f3f (release/2.4)
> Author: Arne Schwabe
> Date:   Wed Jan 31 10:41:02 2018 +0100
> 
>      Treat dhcp-option DNS6 and DNS identical

This is... interesting.  The change is minor, but seems to have pushed the
endless if/else if/else construction in options.c over an invisible cliff,
and some buildbots (FreeBSD 7.4, Debian 8.5) now *sometimes* bomb with
"signal 11" when gcc is compiling options.c ...

  gcc -DHAVE_CONFIG_H -I. -I../.. -I../../include  -I../../include  -I../../src/compat           -DPLUGIN_LIBDIR=\"/usr/local/lib/openvpn/plugins\"  -g -O2 -std=c99 -MT options.o -MD -MP -MF .deps/options.Tpo -c -o options.o options.c
  gcc: Internal error: Killed: 9 (program cc1)
  Please submit a full bug report.
  See <URL:http://gcc.gnu.org/bugs.html> for instructions.


dmesg points to "too much stuff running on that box"

pid 91411 (cc1), uid 1000, was killed: out of swap space

(it's not a general problem - I can compile options.c just fine on f7.4,
but buildbot failed...)

So - if you bump into this, give your system more RAM.  This particular
VM has 512M RAM and *no* swap, seems this is not enough anymore.

Besides this, I don't think we need to do anything about it - except
document this here, so google can find it.

gert

PS: and yes, I know that FreeBSD 7.4 is more ancient than the dawn of
time, but for some reasons I've become fond of it - "my first buildbot!" -
and it's well firewalled off the public Internet...  and it's sort of
my "how far back will it still work?" test.  If we break it, and for good
reason, it will be the XP/Vista thing - I'll try to not break it, but
if there is reasonable API changes that we want to have, goodbye.

Hi,

On Sun, Feb 4, 2018 at 1:01 PM, Gert Doering <gert@greenie.muc.de> wrote:
> Hi,
>
> On Sun, Feb 04, 2018 at 11:50:03AM +0100, Gert Doering wrote:
>> I've taken the liberty to change the commit message and mention that it's
>> "v3" here :-)
>>
>> commit 849006bf17bba524e6f3344598adcbe41bedf450 (master)
>> commit 61a72ecd59a57a50d0757b56b16f4ed3ce700f3f (release/2.4)
>> Author: Arne Schwabe
>> Date:   Wed Jan 31 10:41:02 2018 +0100
>>
>>      Treat dhcp-option DNS6 and DNS identical
>
> This is... interesting.  The change is minor, but seems to have pushed the
> endless if/else if/else construction in options.c over an invisible cliff,
> and some buildbots (FreeBSD 7.4, Debian 8.5) now *sometimes* bomb with
> "signal 11" when gcc is compiling options.c ...
>
>   gcc -DHAVE_CONFIG_H -I. -I../.. -I../../include  -I../../include  -I../../src/compat           -DPLUGIN_LIBDIR=\"/usr/local/lib/openvpn/plugins\"  -g -O2 -std=c99 -MT options.o -MD -MP -MF .deps/options.Tpo -c -o options.o options.c
>   gcc: Internal error: Killed: 9 (program cc1)
>   Please submit a full bug report.
>   See <URL:http://gcc.gnu.org/bugs.html> for instructions.

Interesting indeed. In fact, 2 years ago, we had a report of msvc
failing (literally stack overflow !) on the long if-else chain in
option.c:

quoting from an old thread:

>On Thu, Dec 10, 2015 at 9:47 AM, Selva Nair <selva.nair@gmail.com> wrote:
>>Hi,
>>
>>On Wed, Dec 9, 2015 at 5:05 PM, Gisle Vanem <gvanem@yahoo.no> wrote:
>>
>>>While compiling src/openvpn/options.c with MSVC 2015, I got this
>>>
>>>error:
>>>
>>>  src/openvpn/options.c(5944): fatal error C1026: parser stack overflow, program too complex
>>>
>>>As a quick fix you can locally edit options.c to split the large if ..endif block with 300 or so  else-if's into two groups with a couple of goto's. I've no idea how msvc parses code, so this may or may not work.
>>>
>>>I've also seen such error in GeoIP-lib due to all the 'if..else if' statements.
>>>
>>>Can this be written using some kind of a table-lookup code instead?
>>
>>
>>The option parser code using if-else is very standard and the logic easy to follow, so I doubt there is a strong reason for re-writing it.
>>
>>Selva

At that time I had done some tests. from my notes:

"gcc with 100,000 if else
compiles using
~250 MB memory and ~8 minutes on 1 core of an 8 year old athlon.
So apparently there is no limit except for memory exhaustion."

I can't recall what version of gcc I had used. Its a bit surprising
that we now exhaust memory (even on a VM with only 512 MB memory).
Possibly newer gcc versions are approaching msvc efficiency in this
regard?

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot