[libvirt] [PATCH] qemu: Fix crash on failed VM startup
If VM startup fails early enough (can't find a referenced USB device), libvirtd will crash trying to clear the VNC port bit, since port = 0, which overflows us out of the bitmap bounds. Fix this by being more defensive in the bitmap operations, and only clearing a previously set VNC port. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 2 +- src/util/bitmap.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c8cd50a..f5a1310 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3741,7 +3741,7 @@ retry: if ((vm->def->ngraphics == 1) && vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && vm->def->graphics[0]->data.vnc.autoport && - vm->def->graphics[0]->data.vnc.port != -1) { + vm->def->graphics[0]->data.vnc.port >= QEMU_VNC_PORT_MIN) { if (virBitmapClearBit(driver->reservedVNCPorts, vm->def->graphics[0]->data.vnc.port - \ QEMU_VNC_PORT_MIN) < 0) { diff --git a/src/util/bitmap.c b/src/util/bitmap.c index 69094a5..98e65f8 100644 --- a/src/util/bitmap.c +++ b/src/util/bitmap.c @@ -118,7 +118,7 @@ int virBitmapSetBit(virBitmapPtr bitmap, size_t b) */ int virBitmapClearBit(virBitmapPtr bitmap, size_t b) { - if (b > bitmap->size - 1) + if (bitmap->size != 0 && b > bitmap->size - 1) return -1; bitmap->map[VIR_BITMAP_UNIT_OFFSET(b)] &= ~(1 << VIR_BITMAP_BIT_OFFSET(b)); -- 1.6.6.1
On 06/01/2010 01:10 PM, Cole Robinson wrote:
If VM startup fails early enough (can't find a referenced USB device), libvirtd will crash trying to clear the VNC port bit, since port = 0, which overflows us out of the bitmap bounds.
Fix this by being more defensive in the bitmap operations, and only clearing a previously set VNC port.
+++ b/src/util/bitmap.c @@ -118,7 +118,7 @@ int virBitmapSetBit(virBitmapPtr bitmap, size_t b) */ int virBitmapClearBit(virBitmapPtr bitmap, size_t b) { - if (b > bitmap->size - 1) + if (bitmap->size != 0 && b > bitmap->size - 1)
I think this could use a v2: virBitmapSetBit and virBitmapGetBit should get the same treatment for bounds checking. Meanwhile, we already reject attempts to create a bitmap with SIZE_MAX bits. Therefore, since b is unsigned, we can safely avoid the && and instead do the computation via a single comparison: if (bitmap->size <= b) return -1; For that matter, should virBitmapAlloc(0) return NULL, instead of it's current behavior of allocating an (empty) bitmap? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 06/01/2010 01:10 PM, Cole Robinson wrote:
If VM startup fails early enough (can't find a referenced USB device), libvirtd will crash trying to clear the VNC port bit, since port = 0, which overflows us out of the bitmap bounds.
Fix this by being more defensive in the bitmap operations, and only clearing a previously set VNC port.
+++ b/src/util/bitmap.c @@ -118,7 +118,7 @@ int virBitmapSetBit(virBitmapPtr bitmap, size_t b) */ int virBitmapClearBit(virBitmapPtr bitmap, size_t b) { - if (b > bitmap->size - 1) + if (bitmap->size != 0 && b > bitmap->size - 1)
I think this could use a v2: virBitmapSetBit and virBitmapGetBit should get the same treatment for bounds checking.
Meanwhile, we already reject attempts to create a bitmap with SIZE_MAX bits. Therefore, since b is unsigned, we can safely avoid the && and instead do the computation via a single comparison:
if (bitmap->size <= b) return -1;
For that matter, should virBitmapAlloc(0) return NULL, instead of it's current behavior of allocating an (empty) bitmap?
Yes, you are right - especially since there is no grow operation :-). I should have returned NULL for size 0 request in the original version. Regards, Jim
* src/util/bitmap.c (virBitmapAlloc): Tighten sanity check. ---
For that matter, should virBitmapAlloc(0) return NULL, instead of it's current behavior of allocating an (empty) bitmap?
Yes, you are right - especially since there is no grow operation :-). I should have returned NULL for size 0 request in the original version.
Any objections to this? src/util/bitmap.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/util/bitmap.c b/src/util/bitmap.c index 69094a5..44edb49 100644 --- a/src/util/bitmap.c +++ b/src/util/bitmap.c @@ -1,6 +1,7 @@ /* * bitmap.h: Simple bitmap operations * + * Copyright (C) 2010 Red Hat, Inc. * Copyright (C) 2010 Novell, Inc. * * This library is free software; you can redistribute it and/or @@ -58,7 +59,7 @@ virBitmapPtr virBitmapAlloc(size_t size) virBitmapPtr bitmap; size_t sz; - if (SIZE_MAX - VIR_BITMAP_BITS_PER_UNIT < size) + if (SIZE_MAX - VIR_BITMAP_BITS_PER_UNIT < size || size == 0) return NULL; sz = (size + VIR_BITMAP_BITS_PER_UNIT - 1) / -- 1.7.0.1
Eric Blake wrote:
* src/util/bitmap.c (virBitmapAlloc): Tighten sanity check. ---
For that matter, should virBitmapAlloc(0) return NULL, instead of it's current behavior of allocating an (empty) bitmap?
Yes, you are right - especially since there is no grow operation :-). I should have returned NULL for size 0 request in the original version.
Any objections to this?
src/util/bitmap.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/util/bitmap.c b/src/util/bitmap.c index 69094a5..44edb49 100644 --- a/src/util/bitmap.c +++ b/src/util/bitmap.c @@ -1,6 +1,7 @@ /* * bitmap.h: Simple bitmap operations * + * Copyright (C) 2010 Red Hat, Inc. * Copyright (C) 2010 Novell, Inc. * * This library is free software; you can redistribute it and/or @@ -58,7 +59,7 @@ virBitmapPtr virBitmapAlloc(size_t size) virBitmapPtr bitmap; size_t sz;
- if (SIZE_MAX - VIR_BITMAP_BITS_PER_UNIT < size) + if (SIZE_MAX - VIR_BITMAP_BITS_PER_UNIT < size || size == 0) return NULL;
sz = (size + VIR_BITMAP_BITS_PER_UNIT - 1) /
ACK. Thanks! Jim
On 06/02/2010 01:26 PM, Jim Fehlig wrote:
Eric Blake wrote:
* src/util/bitmap.c (virBitmapAlloc): Tighten sanity check. ---
Yes, you are right - especially since there is no grow operation :-). I should have returned NULL for size 0 request in the original version.
Any objections to this?
ACK.
Thanks; pushed now. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/01/2010 04:03 PM, Eric Blake wrote:
On 06/01/2010 01:10 PM, Cole Robinson wrote:
If VM startup fails early enough (can't find a referenced USB device), libvirtd will crash trying to clear the VNC port bit, since port = 0, which overflows us out of the bitmap bounds.
Fix this by being more defensive in the bitmap operations, and only clearing a previously set VNC port.
+++ b/src/util/bitmap.c @@ -118,7 +118,7 @@ int virBitmapSetBit(virBitmapPtr bitmap, size_t b) */ int virBitmapClearBit(virBitmapPtr bitmap, size_t b) { - if (b > bitmap->size - 1) + if (bitmap->size != 0 && b > bitmap->size - 1)
I think this could use a v2: virBitmapSetBit and virBitmapGetBit should get the same treatment for bounds checking.
Meanwhile, we already reject attempts to create a bitmap with SIZE_MAX bits. Therefore, since b is unsigned, we can safely avoid the && and instead do the computation via a single comparison:
if (bitmap->size <= b) return -1;
For that matter, should virBitmapAlloc(0) return NULL, instead of it's current behavior of allocating an (empty) bitmap?
Thanks, simplified that check and applied it to the other bitmap functions. Updated patch sent. - Cole
On Tue, Jun 01, 2010 at 03:10:19PM -0400, Cole Robinson wrote:
If VM startup fails early enough (can't find a referenced USB device), libvirtd will crash trying to clear the VNC port bit, since port = 0, which overflows us out of the bitmap bounds.
Why is port '0' in the first place ? Don't we always have it initialized to '-1' when autoport is true.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c8cd50a..f5a1310 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3741,7 +3741,7 @@ retry: if ((vm->def->ngraphics == 1) && vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && vm->def->graphics[0]->data.vnc.autoport && - vm->def->graphics[0]->data.vnc.port != -1) { + vm->def->graphics[0]->data.vnc.port >= QEMU_VNC_PORT_MIN) { if (virBitmapClearBit(driver->reservedVNCPorts, vm->def->graphics[0]->data.vnc.port - \ QEMU_VNC_PORT_MIN) < 0) {
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 06/02/2010 05:34 AM, Daniel P. Berrange wrote:
On Tue, Jun 01, 2010 at 03:10:19PM -0400, Cole Robinson wrote:
If VM startup fails early enough (can't find a referenced USB device), libvirtd will crash trying to clear the VNC port bit, since port = 0, which overflows us out of the bitmap bounds.
Why is port '0' in the first place ? Don't we always have it initialized to '-1' when autoport is true.
If autoport is set, port is only shown as -1 when we dump the XML, otherwise it's stored internally as 0. - Cole
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c8cd50a..f5a1310 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3741,7 +3741,7 @@ retry: if ((vm->def->ngraphics == 1) && vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && vm->def->graphics[0]->data.vnc.autoport && - vm->def->graphics[0]->data.vnc.port != -1) { + vm->def->graphics[0]->data.vnc.port >= QEMU_VNC_PORT_MIN) { if (virBitmapClearBit(driver->reservedVNCPorts, vm->def->graphics[0]->data.vnc.port - \ QEMU_VNC_PORT_MIN) < 0) {
Daniel
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake -
Jim Fehlig