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
