Re: dig(1): replace inet_net_pton(3)

2021-01-20 Thread Florian Obser 
On Wed, Jan 20, 2021 at 11:38:40AM +0100, Claudio Jeker wrote:
> On Tue, Jan 19, 2021 at 07:49:29PM +0100, Florian Obser wrote:
> > When we converted isc_sockaddr_t to sockaddr_storage we also moved to
> > inet_net_pton(3). It turns out that was a mistake, at least it's not
> > portable for AF_INET6. Effectively revert that part and hand-roll it
> > using inet_pton(3).
> > 
> > OK?
> 
> I thought inet_net_pton() for AF_INET would still be ok. Handling the

Or we haven't found the problems with AF_INET yet.
The code in lib/libc/net/inet_net_pton is rather grotesque.

The semantics are also mind-boggingly stupid.
 All numbers supplied as "parts" in a `.' notation may be decimal, octal,
 or hexadecimal, as specified in the C language (i.e., a leading 0x or 0X
 implies hexadecimal; otherwise, a leading 0 implies octal; otherwise, the
 number is interpreted as decimal).

"0x0a" is valid and is 10.0.0.0/8
"0x0a0b0c0d" is valid and is 10.11.12.13/32
"0xC0" is valid and is 192.0.0.0/24 because of course the damn thing
understands about network classes.

> AF_INET case by hand here seems rather unpleasant.
> 
> Since this code uses sockaddr to store the result why not use
> getaddrinfo().
> 
> In bgpd the code does more or less this:
> 1. extract the /prefixlen like you do.
> 2. use getnameinfo() on the prefix
> 3. if that fails use inet_net_pton(AF_INET) to parse short forms (like 10/8)

I think inet_net_pton plain needs killing. If we were to agree that a
sensible way to write IPv4 addresses is to use decimal numbers in the
range 0-255 seperated by dots (allowing short form) we could easily
write an AF-independent replacement using getnameinfo.

Nobody uses this except for us it seems. Ports is clean, I found one
hit in the FreeBSD base system in sbin/pfctl/pfctl_parser using only
AF_INET. NetBSD additionally enables it in ftpd.

Anyway, not a hill I'm going to die on.

> 
> Note: I think the bgpd code could also use some further cleanup.
> 
> I'm not sure if it makes sense to force this code to be portable by not
> using inet_net_pton(3). The code is currently non-portable because it uses
> the len fields of the sockaddr structs. So why bother about this function?

I'm restoring previous behaviour.

> In general if we plan to make a portable version of our dig the framework
> can provide a working inet_net_pton(3) version.
> 
> > p.s. it is kinda telling that isc, who introduced the API is (no
> > longer?) using it.
> > 
> > diff --git dighost.c dighost.c
> > index 2d2a52c86e2..2995b7e1602 100644
> > --- dighost.c
> > +++ dighost.c
> > @@ -935,10 +935,16 @@ parse_netprefix(struct sockaddr_storage **sap, int 
> > *plen, const char *value) {
> > struct sockaddr_storage *sa = NULL;
> > struct in_addr *in4;
> > struct in6_addr *in6;
> > -   int prefix_length;
> > +   int prefix_length = -1, i;
> > +   char *sep;
> > +   char buf[INET6_ADDRSTRLEN + sizeof("/128")];
> > +   const char *errstr;
> >  
> > REQUIRE(sap != NULL && *sap == NULL);
> >  
> > +   if (strlcpy(buf, value, sizeof(buf)) >= sizeof(buf))
> > +   fatal("invalid prefix '%s'\n", value);
> > +
> > sa = calloc(1, sizeof(*sa));
> > if (sa == NULL)
> > fatal("out of memory");
> > @@ -952,14 +958,36 @@ parse_netprefix(struct sockaddr_storage **sap, int 
> > *plen, const char *value) {
> > goto done;
> > }
> >  
> > -   if ((prefix_length = inet_net_pton(AF_INET6, value, in6, sizeof(*in6)))
> > -   != -1) {
> > +   sep = strchr(buf, '/');
> > +   if (sep != NULL) {
> > +   *sep++ = '\0';
> > +   prefix_length = strtonum(sep, 0, 128, &errstr);
> > +   if (errstr != NULL)
> > +   fatal("invalid address '%s'", value);
> > +   }
> > +
> > +   if (inet_pton(AF_INET6, buf, in6) == 1) {
> > sa->ss_len = sizeof(struct sockaddr_in6);
> > sa->ss_family = AF_INET6;
> > -   } else if ((prefix_length = inet_net_pton(AF_INET, value, in4,
> > -   sizeof(*in4))) != -1) {
> > +   if (prefix_length > 128 || prefix_length == -1)
> 
> prefix_length can't be bigger than 128. But I see why it makes sense to do
> the same here :)
> 
> > +   prefix_length = 128;
> > +   } else if (inet_pton(AF_INET, buf, in4) == 1) {
> > sa->ss_len = sizeof(struct sockaddr_in);
> > sa->ss_family = AF_INET;
> > +   if (prefix_length > 32 || prefix_length == -1)
> > +   prefix_length = 32;
> 
> In my opinion 10.0.0.0/75 is not a valid address and should not be
> coerced to 10.0.0.0. This should result in an error.
> 
> > +   } else if (prefix_length != -1) {
> > +   if (prefix_length > 32)
> > +   prefix_length = 32;
> 
> Same here.
> 
> > +   for (i = 0; i < 3 ; i++) {
> > +   if (strlcat(buf, ".0", sizeof(buf)) > sizeof(buf))
> > +   fatal("invalid address '%s'", value);
> > +   if (inet_pton(AF_

Re: dig(1): replace inet_net_pton(3)

2021-01-20 Thread Claudio Jeker 
On Tue, Jan 19, 2021 at 07:49:29PM +0100, Florian Obser wrote:
> When we converted isc_sockaddr_t to sockaddr_storage we also moved to
> inet_net_pton(3). It turns out that was a mistake, at least it's not
> portable for AF_INET6. Effectively revert that part and hand-roll it
> using inet_pton(3).
> 
> OK?

I thought inet_net_pton() for AF_INET would still be ok. Handling the
AF_INET case by hand here seems rather unpleasant.

Since this code uses sockaddr to store the result why not use
getaddrinfo().

In bgpd the code does more or less this:
1. extract the /prefixlen like you do.
2. use getnameinfo() on the prefix
3. if that fails use inet_net_pton(AF_INET) to parse short forms (like 10/8)

Note: I think the bgpd code could also use some further cleanup.

I'm not sure if it makes sense to force this code to be portable by not
using inet_net_pton(3). The code is currently non-portable because it uses
the len fields of the sockaddr structs. So why bother about this function?
In general if we plan to make a portable version of our dig the framework
can provide a working inet_net_pton(3) version.

> p.s. it is kinda telling that isc, who introduced the API is (no
> longer?) using it.
> 
> diff --git dighost.c dighost.c
> index 2d2a52c86e2..2995b7e1602 100644
> --- dighost.c
> +++ dighost.c
> @@ -935,10 +935,16 @@ parse_netprefix(struct sockaddr_storage **sap, int 
> *plen, const char *value) {
>   struct sockaddr_storage *sa = NULL;
>   struct in_addr *in4;
>   struct in6_addr *in6;
> - int prefix_length;
> + int prefix_length = -1, i;
> + char *sep;
> + char buf[INET6_ADDRSTRLEN + sizeof("/128")];
> + const char *errstr;
>  
>   REQUIRE(sap != NULL && *sap == NULL);
>  
> + if (strlcpy(buf, value, sizeof(buf)) >= sizeof(buf))
> + fatal("invalid prefix '%s'\n", value);
> +
>   sa = calloc(1, sizeof(*sa));
>   if (sa == NULL)
>   fatal("out of memory");
> @@ -952,14 +958,36 @@ parse_netprefix(struct sockaddr_storage **sap, int 
> *plen, const char *value) {
>   goto done;
>   }
>  
> - if ((prefix_length = inet_net_pton(AF_INET6, value, in6, sizeof(*in6)))
> - != -1) {
> + sep = strchr(buf, '/');
> + if (sep != NULL) {
> + *sep++ = '\0';
> + prefix_length = strtonum(sep, 0, 128, &errstr);
> + if (errstr != NULL)
> + fatal("invalid address '%s'", value);
> + }
> +
> + if (inet_pton(AF_INET6, buf, in6) == 1) {
>   sa->ss_len = sizeof(struct sockaddr_in6);
>   sa->ss_family = AF_INET6;
> - } else if ((prefix_length = inet_net_pton(AF_INET, value, in4,
> - sizeof(*in4))) != -1) {
> + if (prefix_length > 128 || prefix_length == -1)

prefix_length can't be bigger than 128. But I see why it makes sense to do
the same here :)

> + prefix_length = 128;
> + } else if (inet_pton(AF_INET, buf, in4) == 1) {
>   sa->ss_len = sizeof(struct sockaddr_in);
>   sa->ss_family = AF_INET;
> + if (prefix_length > 32 || prefix_length == -1)
> + prefix_length = 32;

In my opinion 10.0.0.0/75 is not a valid address and should not be
coerced to 10.0.0.0. This should result in an error.

> + } else if (prefix_length != -1) {
> + if (prefix_length > 32)
> + prefix_length = 32;

Same here.

> + for (i = 0; i < 3 ; i++) {
> + if (strlcat(buf, ".0", sizeof(buf)) > sizeof(buf))
> + fatal("invalid address '%s'", value);
> + if (inet_pton(AF_INET, buf, in4) == 1) {
> + sa->ss_len = sizeof(struct sockaddr_in);
> + sa->ss_family = AF_INET;
> + goto done;
> + }
> + }
>   } else
>   fatal("invalid address '%s'", value);
>  

-- 
:wq Claudio

dig(1): replace inet_net_pton(3)

2021-01-19 Thread Florian Obser 
When we converted isc_sockaddr_t to sockaddr_storage we also moved to
inet_net_pton(3). It turns out that was a mistake, at least it's not
portable for AF_INET6. Effectively revert that part and hand-roll it
using inet_pton(3).

OK?

p.s. it is kinda telling that isc, who introduced the API is (no
longer?) using it.

diff --git dighost.c dighost.c
index 2d2a52c86e2..2995b7e1602 100644
--- dighost.c
+++ dighost.c
@@ -935,10 +935,16 @@ parse_netprefix(struct sockaddr_storage **sap, int *plen, 
const char *value) {
struct sockaddr_storage *sa = NULL;
struct in_addr *in4;
struct in6_addr *in6;
-   int prefix_length;
+   int prefix_length = -1, i;
+   char *sep;
+   char buf[INET6_ADDRSTRLEN + sizeof("/128")];
+   const char *errstr;
 
REQUIRE(sap != NULL && *sap == NULL);
 
+   if (strlcpy(buf, value, sizeof(buf)) >= sizeof(buf))
+   fatal("invalid prefix '%s'\n", value);
+
sa = calloc(1, sizeof(*sa));
if (sa == NULL)
fatal("out of memory");
@@ -952,14 +958,36 @@ parse_netprefix(struct sockaddr_storage **sap, int *plen, 
const char *value) {
goto done;
}
 
-   if ((prefix_length = inet_net_pton(AF_INET6, value, in6, sizeof(*in6)))
-   != -1) {
+   sep = strchr(buf, '/');
+   if (sep != NULL) {
+   *sep++ = '\0';
+   prefix_length = strtonum(sep, 0, 128, &errstr);
+   if (errstr != NULL)
+   fatal("invalid address '%s'", value);
+   }
+
+   if (inet_pton(AF_INET6, buf, in6) == 1) {
sa->ss_len = sizeof(struct sockaddr_in6);
sa->ss_family = AF_INET6;
-   } else if ((prefix_length = inet_net_pton(AF_INET, value, in4,
-   sizeof(*in4))) != -1) {
+   if (prefix_length > 128 || prefix_length == -1)
+   prefix_length = 128;
+   } else if (inet_pton(AF_INET, buf, in4) == 1) {
sa->ss_len = sizeof(struct sockaddr_in);
sa->ss_family = AF_INET;
+   if (prefix_length > 32 || prefix_length == -1)
+   prefix_length = 32;
+   } else if (prefix_length != -1) {
+   if (prefix_length > 32)
+   prefix_length = 32;
+   for (i = 0; i < 3 ; i++) {
+   if (strlcat(buf, ".0", sizeof(buf)) > sizeof(buf))
+   fatal("invalid address '%s'", value);
+   if (inet_pton(AF_INET, buf, in4) == 1) {
+   sa->ss_len = sizeof(struct sockaddr_in);
+   sa->ss_family = AF_INET;
+   goto done;
+   }
+   }
} else
fatal("invalid address '%s'", value);
 


-- 
I'm not entirely sure you are real.