Testing current CVS on RHEL-5 host exposed a number of problems in the Xen driver - When opening the remote driver for the network / storage / nodedev APIs, we had missed the initialization of several fields, resulting in a crash - Reporting of errors from the remote driver was leaking memory, due to missing xdr_free call. - Small possibility of de-referencing NULL when handling an error from the server, if no error message were provided - Mistakenly tries to activate Xen INotify driver when non-root, even though the directories it needs to monitor are chmod 0700 root.root - Gratuitous reporting of failure to connect to XenD's TCP port when running non-root, even though the proxy / remote driver will suceed - Bad return values from the xenDaemonOpen() method - Double free in the new xenStoreDoListDomains() method probably due to merge error The only really intreresting bit of the patch is for the first issue in the remote driver. I've basically pulled out alot of duplicated code into a couple of helper methods, so make these missing initialization less likely in future. remote_internal.c | 153 +++++++++++++++++++++++++----------------------------- xen_unified.c | 12 ++-- xend_internal.c | 50 ++++++----------- xs_internal.c | 2 4 files changed, 97 insertions(+), 120 deletions(-) Daniel Index: src/remote_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/remote_internal.c,v retrieving revision 1.130 diff -u -p -u -p -r1.130 remote_internal.c --- src/remote_internal.c 28 Jan 2009 16:14:24 -0000 1.130 +++ src/remote_internal.c 28 Jan 2009 16:24:20 -0000 @@ -892,31 +892,70 @@ doRemoteOpen (virConnectPtr conn, goto cleanup; } -static virDrvOpenStatus -remoteOpen (virConnectPtr conn, - virConnectAuthPtr auth, - int flags) +static struct private_data * +remoteAllocPrivateData(virConnectPtr conn) { struct private_data *priv; - int ret, rflags = 0; - - if (inside_daemon) - return VIR_DRV_OPEN_DECLINED; - if (VIR_ALLOC(priv) < 0) { - error (conn, VIR_ERR_NO_MEMORY, _("struct private_data")); - return VIR_DRV_OPEN_ERROR; + virReportOOMError(conn); + return NULL; } if (virMutexInit(&priv->lock) < 0) { error(conn, VIR_ERR_INTERNAL_ERROR, _("cannot initialize mutex")); VIR_FREE(priv); - return VIR_DRV_OPEN_ERROR; + return NULL; } remoteDriverLock(priv); priv->localUses = 1; priv->watch = -1; + priv->sock = -1; + + return priv; +} + +static int +remoteOpenSecondaryDriver(virConnectPtr conn, + virConnectAuthPtr auth, + int flags, + struct private_data **priv) +{ + int ret; + int rflags = 0; + + if (!((*priv) = remoteAllocPrivateData(conn))) + return VIR_DRV_OPEN_ERROR; + + if (flags & VIR_CONNECT_RO) + rflags |= VIR_DRV_OPEN_REMOTE_RO; + rflags |= VIR_DRV_OPEN_REMOTE_UNIX; + + ret = doRemoteOpen(conn, *priv, auth, rflags); + if (ret != VIR_DRV_OPEN_SUCCESS) { + remoteDriverUnlock(*priv); + VIR_FREE(*priv); + } else { + (*priv)->localUses = 1; + remoteDriverUnlock(*priv); + } + + return ret; +} + +static virDrvOpenStatus +remoteOpen (virConnectPtr conn, + virConnectAuthPtr auth, + int flags) +{ + struct private_data *priv; + int ret, rflags = 0; + + if (inside_daemon) + return VIR_DRV_OPEN_DECLINED; + + if (!(priv = remoteAllocPrivateData(conn))) + return VIR_DRV_OPEN_ERROR; if (flags & VIR_CONNECT_RO) rflags |= VIR_DRV_OPEN_REMOTE_RO; @@ -971,7 +1010,6 @@ remoteOpen (virConnectPtr conn, #endif } - priv->sock = -1; ret = doRemoteOpen(conn, priv, auth, rflags); if (ret != VIR_DRV_OPEN_SUCCESS) { conn->privateData = NULL; @@ -3085,30 +3123,13 @@ remoteNetworkOpen (virConnectPtr conn, * which doesn't have its own impl of the network APIs. */ struct private_data *priv; - int ret, rflags = 0; - if (VIR_ALLOC(priv) < 0) { - error (conn, VIR_ERR_NO_MEMORY, _("struct private_data")); - return VIR_DRV_OPEN_ERROR; - } - if (virMutexInit(&priv->lock) < 0) { - error(conn, VIR_ERR_INTERNAL_ERROR, - _("cannot initialize mutex")); - VIR_FREE(priv); - return VIR_DRV_OPEN_ERROR; - } - if (flags & VIR_CONNECT_RO) - rflags |= VIR_DRV_OPEN_REMOTE_RO; - rflags |= VIR_DRV_OPEN_REMOTE_UNIX; - - priv->sock = -1; - ret = doRemoteOpen(conn, priv, auth, rflags); - if (ret != VIR_DRV_OPEN_SUCCESS) { - conn->networkPrivateData = NULL; - VIR_FREE(priv); - } else { - priv->localUses = 1; + int ret; + ret = remoteOpenSecondaryDriver(conn, + auth, + flags, + &priv); + if (ret == VIR_DRV_OPEN_SUCCESS) conn->networkPrivateData = priv; - } return ret; } } @@ -3598,30 +3619,13 @@ remoteStorageOpen (virConnectPtr conn, * which doesn't have its own impl of the network APIs. */ struct private_data *priv; - int ret, rflags = 0; - if (VIR_ALLOC(priv) < 0) { - error (NULL, VIR_ERR_NO_MEMORY, _("struct private_data")); - return VIR_DRV_OPEN_ERROR; - } - if (virMutexInit(&priv->lock) < 0) { - error(conn, VIR_ERR_INTERNAL_ERROR, - _("cannot initialize mutex")); - VIR_FREE(priv); - return VIR_DRV_OPEN_ERROR; - } - if (flags & VIR_CONNECT_RO) - rflags |= VIR_DRV_OPEN_REMOTE_RO; - rflags |= VIR_DRV_OPEN_REMOTE_UNIX; - - priv->sock = -1; - ret = doRemoteOpen(conn, priv, auth, rflags); - if (ret != VIR_DRV_OPEN_SUCCESS) { - conn->storagePrivateData = NULL; - VIR_FREE(priv); - } else { - priv->localUses = 1; + int ret; + ret = remoteOpenSecondaryDriver(conn, + auth, + flags, + &priv); + if (ret == VIR_DRV_OPEN_SUCCESS) conn->storagePrivateData = priv; - } return ret; } } @@ -4551,30 +4555,13 @@ remoteDevMonOpen(virConnectPtr conn, * which doesn't have its own impl of the network APIs. */ struct private_data *priv; - int ret, rflags = 0; - if (VIR_ALLOC(priv) < 0) { - error (NULL, VIR_ERR_NO_MEMORY, _("struct private_data")); - return VIR_DRV_OPEN_ERROR; - } - if (virMutexInit(&priv->lock) < 0) { - error(conn, VIR_ERR_INTERNAL_ERROR, - _("cannot initialize mutex")); - VIR_FREE(priv); - return VIR_DRV_OPEN_ERROR; - } - if (flags & VIR_CONNECT_RO) - rflags |= VIR_DRV_OPEN_REMOTE_RO; - rflags |= VIR_DRV_OPEN_REMOTE_UNIX; - - priv->sock = -1; - ret = doRemoteOpen(conn, priv, auth, rflags); - if (ret != VIR_DRV_OPEN_SUCCESS) { - conn->devMonPrivateData = NULL; - VIR_FREE(priv); - } else { - priv->localUses = 1; + int ret; + ret = remoteOpenSecondaryDriver(conn, + auth, + flags, + &priv); + if (ret == VIR_DRV_OPEN_SUCCESS) conn->devMonPrivateData = priv; - } return ret; } } @@ -6419,6 +6406,7 @@ cleanup: thiscall->err.domain == VIR_FROM_REMOTE && thiscall->err.code == VIR_ERR_RPC && thiscall->err.level == VIR_ERR_ERROR && + thiscall->err.message && STRPREFIX(*thiscall->err.message, "unknown procedure")) { rv = -2; } else { @@ -6426,6 +6414,7 @@ cleanup: &thiscall->err); rv = -1; } + xdr_free((xdrproc_t)xdr_remote_error, (char *)&thiscall->err); } else { rv = 0; } Index: src/xen_unified.c =================================================================== RCS file: /data/cvs/libvirt/src/xen_unified.c,v retrieving revision 1.81 diff -u -p -u -p -r1.81 xen_unified.c --- src/xen_unified.c 27 Jan 2009 08:50:04 -0000 1.81 +++ src/xen_unified.c 28 Jan 2009 16:24:21 -0000 @@ -355,11 +355,13 @@ xenUnifiedOpen (virConnectPtr conn, virC } #if WITH_XEN_INOTIFY - DEBUG0("Trying Xen inotify sub-driver"); - if (drivers[XEN_UNIFIED_INOTIFY_OFFSET]->open(conn, auth, flags) == - VIR_DRV_OPEN_SUCCESS) { - DEBUG0("Activated Xen inotify sub-driver"); - priv->opened[XEN_UNIFIED_INOTIFY_OFFSET] = 1; + if (xenHavePrivilege()) { + DEBUG0("Trying Xen inotify sub-driver"); + if (drivers[XEN_UNIFIED_INOTIFY_OFFSET]->open(conn, auth, flags) == + VIR_DRV_OPEN_SUCCESS) { + DEBUG0("Activated Xen inotify sub-driver"); + priv->opened[XEN_UNIFIED_INOTIFY_OFFSET] = 1; + } } #endif Index: src/xend_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xend_internal.c,v retrieving revision 1.243 diff -u -p -u -p -r1.243 xend_internal.c --- src/xend_internal.c 28 Jan 2009 14:36:23 -0000 1.243 +++ src/xend_internal.c 28 Jan 2009 16:24:21 -0000 @@ -838,9 +838,11 @@ xenDaemonOpen_tcp(virConnectPtr conn, co freeaddrinfo (res); if (!priv->addrlen) { - virReportSystemError(conn, saved_errno, - _("unable to connect to '%s:%s'"), - host, port); + /* Don't raise error when unprivileged, since proxy takes over */ + if (xenHavePrivilege()) + virReportSystemError(conn, saved_errno, + _("unable to connect to '%s:%s'"), + host, port); return -1; } @@ -2721,7 +2723,6 @@ error: * @flags: combination of virDrvOpenFlag(s) * * Creates a localhost Xen Daemon connection - * Note: this doesn't try to check if the connection actually works * * Returns 0 in case of success, -1 in case of error. */ @@ -2730,7 +2731,8 @@ xenDaemonOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, int flags ATTRIBUTE_UNUSED) { - int ret; + char *port = NULL; + int ret = VIR_DRV_OPEN_ERROR; /* Switch on the scheme, which we expect to be NULL (file), * "http" or "xen". @@ -2741,45 +2743,30 @@ xenDaemonOpen(virConnectPtr conn, virXendError(NULL, VIR_ERR_NO_CONNECT, __FUNCTION__); goto failed; } - ret = xenDaemonOpen_unix(conn, conn->uri->path); - if (ret < 0) - goto failed; - - ret = xend_detect_config_version(conn); - if (ret == -1) + if (xenDaemonOpen_unix(conn, conn->uri->path) < 0 || + xend_detect_config_version(conn) == -1) goto failed; } else if (STRCASEEQ (conn->uri->scheme, "xen")) { /* * try first to open the unix socket */ - ret = xenDaemonOpen_unix(conn, "/var/lib/xend/xend-socket"); - if (ret < 0) - goto try_http; - ret = xend_detect_config_version(conn); - if (ret != -1) + if (xenDaemonOpen_unix(conn, "/var/lib/xend/xend-socket") == 0 && + xend_detect_config_version(conn) != -1) goto done; - try_http: /* * try though http on port 8000 */ - ret = xenDaemonOpen_tcp(conn, "localhost", "8000"); - if (ret < 0) - goto failed; - ret = xend_detect_config_version(conn); - if (ret == -1) + if (xenDaemonOpen_tcp(conn, "localhost", "8000") < 0 || + xend_detect_config_version(conn) == -1) goto failed; } else if (STRCASEEQ (conn->uri->scheme, "http")) { - char *port; if (virAsprintf(&port, "%d", conn->uri->port) == -1) goto failed; - ret = xenDaemonOpen_tcp(conn, conn->uri->server, port); - VIR_FREE(port); - if (ret < 0) - goto failed; - ret = xend_detect_config_version(conn); - if (ret == -1) + + if (xenDaemonOpen_tcp(conn, conn->uri->server, port) < 0 || + xend_detect_config_version(conn) == -1) goto failed; } else { virXendError(NULL, VIR_ERR_NO_CONNECT, __FUNCTION__); @@ -2787,10 +2774,11 @@ xenDaemonOpen(virConnectPtr conn, } done: - return(ret); + ret = VIR_DRV_OPEN_SUCCESS; failed: - return(-1); + VIR_FREE(port); + return ret; } Index: src/xs_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xs_internal.c,v retrieving revision 1.85 diff -u -p -u -p -r1.85 xs_internal.c --- src/xs_internal.c 23 Jan 2009 19:18:24 -0000 1.85 +++ src/xs_internal.c 28 Jan 2009 16:24:21 -0000 @@ -597,8 +597,6 @@ xenStoreDoListDomains(xenUnifiedPrivateP ids[ret++] = (int) id; } - free(idlist); - out: VIR_FREE (idlist); return ret; -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|