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_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
>
--
I'm not entirely sure you are real.
