[v2] resolv: add IPv6 support to inet_net_pton()

  Dear all,

The below patch is a new revision to add IPv6 support to
inet_net_pton(). The patch is based on the OpenBSD implementation.

Kind regards,

Job

Signed-off: Job Snijders <job@fastly.com>
---
 resolv/inet_net_pton.c | 69 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 4 deletions(-)
  

* Job Snijders:

> Dear all,
>
> The below patch is a new revision to add IPv6 support to
> inet_net_pton(). The patch is based on the OpenBSD implementation.
>
> Kind regards,
>
> Job
>
> Signed-off: Job Snijders <job@fastly.com>
> ---
>  resolv/inet_net_pton.c | 69 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 65 insertions(+), 4 deletions(-)

I took a step back and looked at the IPv4 implementation.  It does the
weired flexible parsing that inet_aton does, and it tries to infer the
network class if no prefix is given.  (Classes are obsolete since CIDR
was introduced in the early 90s.)

Contrast this with inet_pton (the non-network variant), which does not
handle the obsolete inet_aton number formats.  Your inet_net_pton
implementation for IPv6 is necessarily inconsistent, too, because it
cannot infer classes (they do not exist for IPv6), so a bare address is
treated as a /128 network.  Your implementation is also inconsistent
with the OpenBSD implementation, which is documented with this note:

| when the number of bits is specified using “/bits” notation, the value
| of the address still includes all bits supplied in the external
| representation, even those bits which are the host part of an Internet
| address.

I think the interface would be more useful if it behaved consistently
across IPv4 and IPv6.  So “192.192.0.1” should be a 32-bit prefix, and
“192.192.0.1/24” should write the full 8 bytes to the destination.  (All
excess bytes not written should probably be zeroed.)  The inet_net_ntop
function would have to be adjusted accordingly.

We can preserve backwards compatibility for existing applications if we
introduce a new symbol version.  We need to do this anyway eventually
because we want to move the symbol from libresolv to libc.

Do you want to work on this?  I think the documentation will be the
largest part of a patch.

Thanks,
Florian
  

On 4/16/24 12:59, Job Snijders wrote:
> Dear all,
> 
> The below patch is a new revision to add IPv6 support to
> inet_net_pton(). The patch is based on the OpenBSD implementation.
> 
> Kind regards,
> 
> Job
> 
> Signed-off: Job Snijders <job@fastly.com>

In my context as a project steward, and because DJ asked me to look at this patch
with regards to the license changes, I'm going to ask some clarifying questions to
make sure we respect your wishes as the contributor of the patch.

For the review I will note that:

(a) You are using DCO...

> ---
>  resolv/inet_net_pton.c | 69 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 65 insertions(+), 4 deletions(-)
> 
> diff --git a/resolv/inet_net_pton.c b/resolv/inet_net_pton.c
> index 63a47b7394..93753cb37f 100644
> --- a/resolv/inet_net_pton.c
> +++ b/resolv/inet_net_pton.c
> @@ -1,4 +1,6 @@
>  /*
> + * Copyright (c) 2024 Job Snijders <job@fastly.com>
> + * Copyright (c) 2012 by Gilles Chehade <gilles@openbsd.org>

... and (b) You add new copyright notices for specific individuals to the ISC
license portion.

Now the questions:

(1) May you please confirm that you are contributing your own changes under
    the ISC License?

    - Gilles Chehade <gilles@openbsd.org> changes were already contributed
      under that license to OpenBSD.

    - The other acceptable license in this case, and it's up to you, would
      be LGPLv2.1+. The choice here is a balance between what glibc
      will accept e.g. compatible free license, and what you want to
      contribute under. If you are considering keeping this code in sync
      with OpenBSD, then it would be simpler to stay with the ISC License
      that we already have here.

(2) Would you be amenable to using "Copyright The GNU Toolchain Authors."
    as your notice instead?

    - You are absolutely entitled to a copyright notice if you request
      it, but glibc project maintenance is simplified by a using a general
      notice.
      Please see: https://www.linuxfoundation.org/blog/blog/copyright-notices-in-open-source-software-projects

    - The notice from Gilles Chehade must be preserved because it comes
      from the copy of the OpenBSD implementation.

Thank you for contributing to glibc! :-)

I know Florian had some further down-thread suggestions, but the license
and copyright issues still apply.

>   * Copyright (c) 1996,1999 by Internet Software Consortium.
>   *
>   * Permission to use, copy, modify, and distribute this software for any
> @@ -19,6 +21,7 @@
>  #include <sys/socket.h>
>  #include <netinet/in.h>
>  #include <arpa/inet.h>
> +#include <resolv/resolv-internal.h>
>  
>  #include <assert.h>
>  #include <ctype.h>
> @@ -35,13 +38,16 @@
>  
>  static int	inet_net_pton_ipv4 (const char *src, u_char *dst,
>  				    size_t size) __THROW;
> +static int	inet_net_pton_ipv6 (const char *src, u_char *dst,
> +				    size_t size) __THROW;
>  
>  /*
> - * static int
> + * int
>   * inet_net_pton(af, src, dst, size)
> - *	convert network number from presentation to network format.
> - *	accepts hex octets, hex strings, decimal octets, and /CIDR.
> - *	"size" is in bytes and describes "dst".
> + *	Convert network number from presentation format to network format.
> + *	If "af" is set to AF_INET, accept various formats like hex octets,
> + *	hex strings, or decimal octets. If "af" is set to AF_INET6, accept
> + *	IPv6 addresses. "size" is in bytes and describes "dst".
>   * return:
>   *	number of bits, either imputed classfully or specified with /CIDR,
>   *	or -1 if some failure occurred (check errno).  ENOENT means it was
> @@ -55,6 +61,8 @@ inet_net_pton (int af, const char *src, void *dst, size_t size)
>  	switch (af) {
>  	case AF_INET:
>  		return (inet_net_pton_ipv4(src, dst, size));
> +	case AF_INET6:
> +		return (inet_net_pton_ipv6(src, dst, size));
>  	default:
>  		__set_errno (EAFNOSUPPORT);
>  		return (-1);
> @@ -196,3 +204,56 @@ inet_net_pton_ipv4 (const char *src, u_char *dst, size_t size)
>  	__set_errno (EMSGSIZE);
>  	return (-1);
>  }
> +
> +
> +/*
> + * Convert an IPv6 prefix from presentation format to network format.
> + * Return the number of bits specified, or -1 as error (check errno).
> + */
> +static int
> +inet_net_pton_ipv6 (const char *src, u_char *dst, size_t size)
> +{
> +	struct in6_addr	 in6;
> +	int		 bits;
> +	long		 lbits;
> +	size_t		 bytes;
> +	char		*ep, *sep;
> +
> +	sep = strchr(src, '/');
> +
> +	if (__inet_pton_length(AF_INET6, src, sep ? sep - src : strlen(src),
> +	    &in6) != 1) {
> +		__set_errno (ENOENT);
> +		return (-1);
> +	}
> +
> +	if (sep == NULL) {
> +		bits = 128;
> +		goto out;
> +	}
> +
> +	if (sep[0] == '\0' || !isascii(sep[0]) || !isdigit(sep[0])) {
> +		__set_errno (ENOENT);
> +		return (-1);
> +	}
> +
> +	lbits = strtol(sep, &ep, 10);
> +	if (ep == sep || *ep != '\0') {
> +		__set_errno (ENOENT);
> +		return (-1);
> +	}
> +	if (lbits < 0 || lbits > 128) {
> +		__set_errno (EMSGSIZE);
> +		return (-1);
> +	}
> +	bits = lbits;
> +
> + out:
> +	bytes = (bits + 7) / 8;
> +	if (bytes > size) {
> +		__set_errno (EMSGSIZE);
> +		return (-1);
> +	}
> +	memcpy(dst, &in6.s6_addr, bytes);
> +	return (bits);
> +}
>
  

Dear Carlos,

On Fri, 3 May 2024 at 17:32, Carlos O'Donell <carlos@redhat.com> wrote:

> On 4/16/24 12:59, Job Snijders wrote:
> > Dear all,
> >
> > The below patch is a new revision to add IPv6 support to
> > inet_net_pton(). The patch is based on the OpenBSD implementation.
> >
> > Kind regards,
> >
> > Job
> >
> > Signed-off: Job Snijders <job@fastly.com>
>
> In my context as a project steward, and because DJ asked me to look at
> this patch
> with regards to the license changes, I'm going to ask some clarifying
> questions to
> make sure we respect your wishes as the contributor of the patch.
>
> For the review I will note that:
>
> (a) You are using DCO...



What does “DCO” mean?


> ---
> >  resolv/inet_net_pton.c | 69 +++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 65 insertions(+), 4 deletions(-)
> >
> > diff --git a/resolv/inet_net_pton.c b/resolv/inet_net_pton.c
> > index 63a47b7394..93753cb37f 100644
> > --- a/resolv/inet_net_pton.c
> > +++ b/resolv/inet_net_pton.c
> > @@ -1,4 +1,6 @@
> >  /*
> > + * Copyright (c) 2024 Job Snijders <job@fastly.com>
> > + * Copyright (c) 2012 by Gilles Chehade <gilles@openbsd.org>
>
> ... and (b) You add new copyright notices for specific individuals to the
> ISC
> license portion.
>
> Now the questions:
>
> (1) May you please confirm that you are contributing your own changes under
>     the ISC License?



Yes, confirmed.


    - Gilles Chehade <gilles@openbsd.org> changes were already contributed
>       under that license to OpenBSD.
>
>     - The other acceptable license in this case, and it's up to you, would
>       be LGPLv2.1+. The choice here is a balance between what glibc
>       will accept e.g. compatible free license, and what you want to
>       contribute under. If you are considering keeping this code in sync
>       with OpenBSD, then it would be simpler to stay with the ISC License
>       that we already have here.
>
> (2) Would you be amenable to using "Copyright The GNU Toolchain Authors."
>     as your notice instead?



I’d prefer to be listed by my own name.


    - You are absolutely entitled to a copyright notice if you request
>       it, but glibc project maintenance is simplified by a using a general
>       notice.
>       Please see:
> https://www.linuxfoundation.org/blog/blog/copyright-notices-in-open-source-software-projects
>
>     - The notice from Gilles Chehade must be preserved because it comes
>       from the copy of the OpenBSD implementation.
>
> Thank you for contributing to glibc! :-)



You are welcome, thanks for taking a look!

Kind regards,

Job
  

Job Snijders <job@fastly.com> writes:
>  (a) You are using DCO...
>
> What does “DCO” mean?

Developer Certificate of Origin.  I.e. it's what youre signing-off on.

https://sourceware.org/glibc/wiki/dco
  

> Developer Certificate of Origin.  I.e. it's what youre signing-off on.
> 
> https://sourceware.org/glibc/wiki/dco

In context:

https://sourceware.org/glibc/wiki/Contribution%20checklist#Developer_Certificate_of_Origin