2:10 p.m.
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(a)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
3:03 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:40 p.m.
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
10:05 a.m.
New subject: [libvirt] [PATCH] bitmap: reject zero-size bitmap
* 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
2:26 p.m.
New subject: [libvirt] [PATCH] bitmap: reject zero-size bitmap
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
3:18 p.m.
New subject: [libvirt] [PATCH] bitmap: reject zero-size bitmap
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:55 a.m.
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
4:34 a.m.
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 :|
9:34 a.m.
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
5263
days inactive
5264
days old
8 comments
4 participants
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake -
Jim Fehlig