[libvirt] [PATCH] BZ1072677: Avoid freeing of 0 file descriptor
From: Stefan Berger <stefanb@linux.vnet.ibm.com> Avoid the freeing of an array of zero file descriptors in case of error. Introduce a macro VIR_INIT_N_FD to initialize such an array's elements to -1. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/qemu/qemu_hotplug.c | 14 +++++++++++--- src/util/virfile.h | 12 ++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6703c92..b295db2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -874,9 +874,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, tapfdSize = vhostfdSize = net->driver.virtio.queues; if (!tapfdSize) tapfdSize = vhostfdSize = 1; - if (VIR_ALLOC_N(tapfd, tapfdSize) < 0 || - VIR_ALLOC_N(vhostfd, vhostfdSize) < 0) + if (VIR_ALLOC_N(tapfd, tapfdSize) < 0) goto cleanup; + VIR_INIT_N_FD(tapfd, tapfdSize); + if (VIR_ALLOC_N(vhostfd, vhostfdSize) < 0) + goto cleanup; + VIR_INIT_N_FD(vhostfd, vhostfdSize); if (qemuNetworkIfaceConnect(vm->def, conn, driver, net, priv->qemuCaps, tapfd, &tapfdSize) < 0) goto cleanup; @@ -885,8 +888,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { tapfdSize = vhostfdSize = 1; - if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(vhostfd) < 0) + if (VIR_ALLOC(tapfd) < 0) + goto cleanup; + *tapfd = -1; + if (VIR_ALLOC(vhostfd) < 0) goto cleanup; + *vhostfd = -1; if ((tapfd[0] = qemuPhysIfaceConnect(vm->def, driver, net, priv->qemuCaps, VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0) @@ -898,6 +905,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, vhostfdSize = 1; if (VIR_ALLOC(vhostfd) < 0) goto cleanup; + *vhostfd = -1; if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; } diff --git a/src/util/virfile.h b/src/util/virfile.h index 20baf6f..802cf01 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -75,6 +75,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; VIR_FILE_CLOSE_PRESERVE_ERRNO | \ VIR_FILE_CLOSE_DONT_LOG)) +static inline void vir_init_n_int(int *ptr, int count, int value) +{ + int i; + + for (i = 0; i < count; i++) + ptr[i] = value; +} + +/* Initialize an array of file descriptors to -1 */ +# define VIR_INIT_N_FD(ptr, count) \ + vir_init_n_int(ptr, count, -1) + /* Opaque type for managing a wrapper around a fd. */ struct _virFileWrapperFd; -- 1.8.1.4
On 03/08/2014 04:29 PM, Stefan Berger wrote:
From: Stefan Berger <stefanb@linux.vnet.ibm.com>
Avoid the freeing of an array of zero file descriptors in case of error. Introduce a macro VIR_INIT_N_FD to initialize such an array's elements to -1.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/qemu/qemu_hotplug.c | 14 +++++++++++--- src/util/virfile.h | 12 ++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-)
+++ b/src/util/virfile.h @@ -75,6 +75,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; VIR_FILE_CLOSE_PRESERVE_ERRNO | \ VIR_FILE_CLOSE_DONT_LOG))
+static inline void vir_init_n_int(int *ptr, int count, int value) +{ + int i; + + for (i = 0; i < count; i++) + ptr[i] = value; +}
Do we ever plan on using this for values other than '-1'?
+ +/* Initialize an array of file descriptors to -1 */ +# define VIR_INIT_N_FD(ptr, count) \ + vir_init_n_int(ptr, count, -1)
Could also be spelled: memset(ptr, -1, sizeof(*ptr) * count)) which goes back to why we need vir_init_n_int(). I agree that the fix to qemu_hotplug.c to not close fd 0 on failure is needed, but am not sure about the complexity of the virfile.h addition. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> wrote on 03/10/2014 02:09:58 PM:
From: Eric Blake <eblake@redhat.com> To: Stefan Berger/Watson/IBM@IBMUS, libvir-list@redhat.com, Cc: laine@laine.org Date: 03/10/2014 02:24 PM Subject: Re: [libvirt] [PATCH] BZ1072677: Avoid freeing of 0 file descriptor
On 03/08/2014 04:29 PM, Stefan Berger wrote:
From: Stefan Berger <stefanb@linux.vnet.ibm.com>
Avoid the freeing of an array of zero file descriptors in case of error. Introduce a macro VIR_INIT_N_FD to initialize such an array's elements to -1.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/qemu/qemu_hotplug.c | 14 +++++++++++--- src/util/virfile.h | 12 ++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-)
+++ b/src/util/virfile.h @@ -75,6 +75,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; VIR_FILE_CLOSE_PRESERVE_ERRNO | \ VIR_FILE_CLOSE_DONT_LOG))
+static inline void vir_init_n_int(int *ptr, int count, int value) +{ + int i; + + for (i = 0; i < count; i++) + ptr[i] = value; +}
Do we ever plan on using this for values other than '-1'?
It's probably too general for the fd purpose here only.
+ +/* Initialize an array of file descriptors to -1 */ +# define VIR_INIT_N_FD(ptr, count) \ + vir_init_n_int(ptr, count, -1)
Could also be spelled:
memset(ptr, -1, sizeof(*ptr) * count))
which goes back to why we need vir_init_n_int().
Also this would be possible.
I agree that the fix to qemu_hotplug.c to not close fd 0 on failure is needed, but am not sure about the complexity of the virfile.h addition.
So a simple memset() would do? Having a macro for VIR_ALLOC_N_FD() would probably be too much, eh? Stefan
On 03/10/2014 01:10 PM, Stefan Berger wrote:
Eric Blake <eblake@redhat.com> wrote on 03/10/2014 02:09:58 PM:
+/* Initialize an array of file descriptors to -1 */ +# define VIR_INIT_N_FD(ptr, count) \ + vir_init_n_int(ptr, count, -1)
Could also be spelled:
memset(ptr, -1, sizeof(*ptr) * count))
which goes back to why we need vir_init_n_int().
Also this would be possible.
I agree that the fix to qemu_hotplug.c to not close fd 0 on failure is needed, but am not sure about the complexity of the virfile.h addition.
So a simple memset() would do? Having a macro for VIR_ALLOC_N_FD() would probably be too much, eh?
Indeed: $ git grep memset.*1 src/conf/domain_conf.c: memset(max_idx, -1, sizeof(max_idx)); src/qemu/qemu_command.c: memset(tapfd, -1, tapfdSize * sizeof(tapfd[0])); src/qemu/qemu_command.c: memset(vhostfd, -1, vhostfdSize * sizeof(vhostfd[0])); So let's just copy that convention, instead of adding a new macro. ACK to the qemu_hotplug fixes if you switch to raw memset. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> wrote on 03/10/2014 05:49:54 PM:
Indeed:
$ git grep memset.*1 src/conf/domain_conf.c: memset(max_idx, -1, sizeof(max_idx)); src/qemu/qemu_command.c: memset(tapfd, -1, tapfdSize * sizeof(tapfd[0])); src/qemu/qemu_command.c: memset(vhostfd, -1, vhostfdSize * sizeof(vhostfd[0]));
So let's just copy that convention, instead of adding a new macro.
ACK to the qemu_hotplug fixes if you switch to raw memset.
Pushed.
On 09.03.2014 00:29, Stefan Berger wrote:
From: Stefan Berger <stefanb@linux.vnet.ibm.com>
Avoid the freeing of an array of zero file descriptors in case of error. Introduce a macro VIR_INIT_N_FD to initialize such an array's elements to -1.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> ---
Just a small nitpick, I'd reword the subject (something among 'qemu_hotplug: avoid freeing ..' or even s/qemu_hotplug/qemuDomainAttachNetDevice/). And then I'd put link to the bug on the first line of the commit message. Michal
participants (3)
-
Eric Blake -
Michal Privoznik -
Stefan Berger