Libguestfs - Oct 2020 - Re: [xml] Why does libxml2 limit port numbers to 999, 999, 999?

The AF_VSOCK protocol added in Linux 5.6 uses a 32 bit port number.

For NBD we map this to simple URIs[1] like nbd+vsock://CID:PORT (where
CID is a number that acts a bit like a hostname and PORT is a 32 bit
port number).  eg: nbd+vsock://1:1000000000/ would be port 10^9 on the
loopback address VMADDR_CID_LOCAL (== 1).

The problem is that libxml2 arbitrarily limits port numbers to
999,999,999.  I don't see any support for this limit in RFC 3986 [2].

Here is the code:

 
https://github.com/GNOME/libxml2/blob/46837d47d59c7b8c9bd1d08a6a717a90a7f1ceb6/uri.c#L333

It doesn't even return an error, just truncates the port number.  You
can see the problem with the program [3] below.

It seems like libxml2 chose to do this for convenience rather than
correctness.  I think it should accept port numbers at least up to
signed int (the type used to store port numbers), and give an error if
the port number overflows.

Is there anything I'm missing or would a patch which implements that
be acceptable?

Also could the uri->port field be changed to unsigned int without
breaking ABI?

Rich.

[1] https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md

[2] https://tools.ietf.org/html/rfc3986#section-3.2.3

[3]
  $ cat port.c
  #include <stdio.h>
  #include <stdlib.h>
  #include <stdint.h>
  #include <inttypes.h>
  #include <libxml/uri.h>

  int
  main (int argc, char *argv[])
  {
    xmlURIPtr uri = xmlParseURI (argv[1]);
    if (!uri) {
      fprintf (stderr, "xmlParseURI failed\n");
      exit (EXIT_FAILURE);
    }
    printf ("%s => uri->port = %d\n", argv[1], uri->port);
    exit (EXIT_SUCCESS);
  }

  $ ./port nbd+vsock://1:1000
  nbd+vsock://1:1000 => uri->port = 1000
  $ ./port nbd+vsock://1:100000
  nbd+vsock://1:100000 => uri->port = 100000
  $ ./port nbd+vsock://1:10000000
  nbd+vsock://1:10000000 => uri->port = 10000000
  $ ./port nbd+vsock://1:1000000000
  nbd+vsock://1:1000000000 => uri->port = 99999999


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Sat, Oct 17, 2020 at 11:24:31AM +0100, Richard W.M. Jones via xml
wrote:
> The problem is that libxml2 arbitrarily limits port numbers to
> 999,999,999.  I don't see any support for this limit in RFC 3986 [2].
Actually I miscounted the number of 9's.  The limit is 99,999,999.
The rest still applies.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Oct 17, 2020, at 12:24 , Richard W.M. Jones via xml <xml@gnome.org>
wrote:
> It seems like libxml2 chose to do this for convenience rather than
> correctness.
Yes, this is an arbitrary limit introduced to avoid integer overflow.
 
> I think it should accept port numbers at least up to
> signed int (the type used to store port numbers), and give an error if
> the port number overflows.
This is fixed now:
https://gitlab.gnome.org/GNOME/libxml2/-/commit/b46016b8705b041c0678dd45e445dc73674b75d0
> Also could the uri->port field be changed to unsigned int without
> breaking ABI?
It’s a public struct member, so strictly speaking, no. But the risk to break
stuff seems low.

Nick

On Sat, Oct 17, 2020 at 06:32:18PM +0200, Nick Wellnhofer
wrote:
> On Oct 17, 2020, at 12:24 , Richard W.M. Jones via xml
<xml@gnome.org> wrote:
> > It seems like libxml2 chose to do this for convenience rather than
> > correctness.
> 
> Yes, this is an arbitrary limit introduced to avoid integer overflow.
>  
> > I think it should accept port numbers at least up to
> > signed int (the type used to store port numbers), and give an error if
> > the port number overflows.
> 
> This is fixed now:
https://gitlab.gnome.org/GNOME/libxml2/-/commit/b46016b8705b041c0678dd45e445dc73674b75d0
Oh that's great thanks.  Can confirm it works for me (up to INT_MAX).
> > Also could the uri->port field be changed to unsigned int without
> > breaking ABI?
>
> It’s a public struct member, so strictly speaking, no. But the risk
> to break stuff seems low.
This would allow us to go to 2^32-1 which is the full range of
port numbers for AF_VSOCK.

** Stefano ** Do you think this is worth it for the vsock protocol?
I'm not sure how often huge port numbers are used - I only hit this
bug because I was choosing random port numbers in a test case.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org