[libvirt] [PATCH 2/2] Fix race in finding available vnc port

[Stefan Berger]

On Mon, 2010-05-17 at 12:10 -0600, Jim Fehlig wrote:
> The qemu driver contains a subtle race in the logic to find next
> available vnc port.  Currently it iterates through all available ports
> and returns the first for which bind(2) succeeds.  However it is possible
> that a previously issued port has not yet been bound by qemu, resulting
> in the same port used for a subsequent domain.
> 
> This patch addresses the race by using a simple bitmap to "reserve" the
> ports allocated by libvirt.
> ---
>  src/qemu/qemu_driver.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 582fdee..329859e 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2629,13 +2629,24 @@ qemuInitPCIAddresses(struct qemud_driver *driver,
>      return ret;
>  }
> 
> +static virBitmapPtr vnc_used_port_bitmap;
> +
>  static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) {
>      int i;
> 
> +    if (vnc_used_port_bitmap == NULL) {
> +        if ((vnc_used_port_bitmap = virBitmapAlloc(59635)) == NULL)
> +            return -1;
> +    }
> +
I think you could move the above into qmudStartup.

Maybe replace the 59635 with (VNC_PORT_MAX - VNC_PORT_MIN) so one can
see the math ...

>      for (i = 5900 ; i < 65535 ; i++) {

... and replace 5900 here with VNC_PORT_MIN and 65535 with VNC_PORT_MAX.

>          int fd;
>          int reuse = 1;
>          struct sockaddr_in addr;
> +
> +        if (virBitmapGetBit(vnc_used_port_bitmap, i - 5900))
> +            continue;
> +
>          addr.sin_family = AF_INET;
>          addr.sin_port = htons(i);
>          addr.sin_addr.s_addr = htonl(INADDR_ANY);
> @@ -2651,6 +2662,8 @@ static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) {
>          if (bind(fd, (struct sockaddr*)&addr, sizeof(addr)) == 0) {
>              /* Not in use, lets grab it */
>              close(fd);
> +            /* Add port to bitmap of reserved ports */
> +            virBitmapSetBit(vnc_used_port_bitmap, i - 5900);
>              return i;
>          }
>          close(fd);
> @@ -3717,6 +3730,15 @@ retry:
> 
>      qemudRemoveDomainStatus(driver, vm);
> 
> +    /* Remove VNC port from vnc_used_port_bitmap, but only if it was reserved
> +       by the driver (autoport=yes)
> +    */
> +    if ((vm->def->ngraphics == 1) &&
> +        vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
> +        vm->def->graphics[0]->data.vnc.autoport)
> +       virBitmapClearBit(vnc_used_port_bitmap,
> +                         vm->def->graphics[0]->data.vnc.port - 5900);
> +
>      vm->pid = -1;
>      vm->def->id = -1;
>      vm->state = VIR_DOMAIN_SHUTOFF;


Afaict, you also need to add cleanup to the 'cleanup:' part of
qemudStartVMDaemon() after checking that the port has been allocated.

   Stefan