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

[Jim Fehlig]

Stefan Berger wrote:
> 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.
>   

I originally had it there but decided it was a waste of 64k when no
domains use vnc :-).  I'll move it back.

> 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.
>   

Right.  Probably another patch that defines VNC_PORT_{MIN,MAX} and use
those in the existing for loop, followed by this patch.

>   
>>          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.
>   

Nod.  I'll respin these patches.

Jim