On 02/09/2012 09:43 AM, Martin Kletzander wrote: The problem alluded to here is that when xmlParseURI encounters a numeric IPv6 address in brackets, it returns the entire string including the brackets. getaddrxx(), by contrast, expects the brackets to not be there. And yet the brackets *must* be included to specify a numeric IP address, according to section 6 of RFC5952 (why am I ready with this information? Because I looked it up when commenting on a qemu bug last night :-) (https://bugzilla.redhat.com/show_bug.cgi?id=680356 if you're interested) I almost ACKed this patch (with one small change to replace the loop with memmove), but then thought about what happens if we need to reconstruct the URI from this modified xmlURIPtr (e.g., with xmlSaveUri, which libvirt calls in two places). Since we've modified the server string, calling xmlSaveUri will give us incorrect results - let's say you start off with: http://[2098:2da:335b::1]:123 The original xmlParseURI would return with server == "[2098:2da:335b::1]", and port == 123. If we called xmlSaveUri on that, we would get back the original string. But if we have modified the server string to remove the brackets, calling xmlSaveUri gives us: http://2098:2da:335b::1:123 Which is a completely different address. So, I think that the correct solution here, rather than permanently modifying the server string returned from xmlParseURI, is to look at the places where the server string is used for something other than just checking for != NULL (that leaves very few places) and modify a copy of the string to remove the brackets in those cases. This way you always have the exact original server string so that the original URI can be reconstructed. So, NACK. (note that I still made some comments on the code that modifies server, which you'll want to look at before making a helper function to return an unbracketed copy (or however you end up fixing it without modifying the original). I've never found a document that said [domainname]:port was legal, so I guess it's okay to check for at least one ":" in the string. I would have probably left that out though, as I'm fairly certain that "[" and "]" still aren't allowed in domain names (even after the expansion to support UTF characters). (this actually copies one character too many, but that's harmless). Just replace this with: memmove(ret->server, ret->server+1, last-1)

Function xmlParseURI does not remove square brackets around IPv6 address when parsing. One of the solutions is making wrappers around functions working with xmlURI*. This assures that uri->server will be always properly assigned and it doesn't have to be changed when used on some new place in the code. For this purpose, functions virParseURI and virSaveURI were added. These function are wrappers around xmlParseURI and xmlSaveUri respectively. File changes: - src/util/xml.h -- declaration - src/util/xml.c -- definition - src/libvirt_private.syms -- symbol export - all others -- function call fixed and include added --- v2: - added virSaveURI for building back the original string src/esx/esx_driver.c | 3 +- src/libvirt.c | 7 ++- src/libvirt_private.syms | 2 + src/libxl/libxl_driver.c | 3 +- src/lxc/lxc_driver.c | 3 +- src/openvz/openvz_driver.c | 3 +- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_migration.c | 5 +- src/remote/remote_driver.c | 5 +- src/uml/uml_driver.c | 3 +- src/util/xml.c | 94 ++++++++++++++++++++++++++++++++++++++++++++ src/util/xml.h | 5 ++ src/vbox/vbox_tmpl.c | 3 +- src/vmx/vmx.c | 3 +- src/xen/xen_driver.c | 2 +- src/xen/xend_internal.c | 3 +- 16 files changed, 129 insertions(+), 17 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index f5e1cc7..4375432 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -44,6 +44,7 @@ #include "esx_vi.h" #include "esx_vi_methods.h" #include "esx_util.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_ESX @@ -3976,7 +3977,7 @@ esxDomainMigratePerform(virDomainPtr domain, } /* Parse migration URI */ - parsedUri = xmlParseURI(uri); + parsedUri = virParseURI(uri); if (parsedUri == NULL) { virReportOOMError(); diff --git a/src/libvirt.c b/src/libvirt.c index 6294196..465d0aa 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -44,6 +44,7 @@ #include "command.h" #include "virnodesuspend.h" #include "virrandom.h" +#include "xml.h" #ifndef WITH_DRIVER_MODULES # ifdef WITH_TEST @@ -1127,7 +1128,7 @@ do_open (const char *name, virConnectOpenResolveURIAlias(name, &alias) < 0) goto failed; - ret->uri = xmlParseURI (alias ? alias : name); + ret->uri = virParseURI (alias ? alias : name); if (!ret->uri) { virLibConnError(VIR_ERR_INVALID_ARG, _("could not parse connection URI %s"), @@ -1729,7 +1730,7 @@ virConnectGetURI (virConnectPtr conn) return NULL; } - name = (char *)xmlSaveUri(conn->uri); + name = (char *)virSaveURI(conn->uri); if (!name) { virReportOOMError(); goto error; @@ -4964,7 +4965,7 @@ virDomainMigratePeer2Peer (virDomainPtr domain, return -1; } - tempuri = xmlParseURI(dconnuri); + tempuri = virParseURI(dconnuri); if (!tempuri) { virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); virDispatchError(domain->conn); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9e3573f..595020a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1431,6 +1431,8 @@ virTypedParameterAssign; # xml.h +virParseURI; +virSaveURI; virXMLChildElementCount; virXMLParseHelper; virXMLPropString; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 6cfc5eb..f1ef5fb 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -44,6 +44,7 @@ #include "libxl_conf.h" #include "xen_xm.h" #include "virtypedparam.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_LIBXL @@ -1043,7 +1044,7 @@ libxlOpen(virConnectPtr conn, if (libxl_driver == NULL) return VIR_DRV_OPEN_DECLINED; - conn->uri = xmlParseURI("xen:///"); + conn->uri = virParseURI("xen:///"); if (!conn->uri) { virReportOOMError(); return VIR_DRV_OPEN_ERROR; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index b6962cf7..3d25d5e 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -60,6 +60,7 @@ #include "virnodesuspend.h" #include "virtime.h" #include "virtypedparam.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -139,7 +140,7 @@ static virDrvOpenStatus lxcOpen(virConnectPtr conn, if (lxc_driver == NULL) return VIR_DRV_OPEN_DECLINED; - conn->uri = xmlParseURI("lxc:///"); + conn->uri = virParseURI("lxc:///"); if (!conn->uri) { virReportOOMError(); return VIR_DRV_OPEN_ERROR; diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 833a98d..47fad74 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -56,6 +56,7 @@ #include "virfile.h" #include "logging.h" #include "command.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_OPENVZ @@ -1335,7 +1336,7 @@ static virDrvOpenStatus openvzOpen(virConnectPtr conn, if (access("/proc/vz", W_OK) < 0) return VIR_DRV_OPEN_DECLINED; - conn->uri = xmlParseURI("openvz:///system"); + conn->uri = virParseURI("openvz:///system"); if (conn->uri == NULL) { virReportOOMError(); return VIR_DRV_OPEN_ERROR; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 717bdf1..3dcf461 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -857,7 +857,7 @@ static virDrvOpenStatus qemudOpen(virConnectPtr conn, if (qemu_driver == NULL) return VIR_DRV_OPEN_DECLINED; - conn->uri = xmlParseURI(qemu_driver->privileged ? + conn->uri = virParseURI(qemu_driver->privileged ? "qemu:///system" : "qemu:///session"); if (!conn->uri) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f0af494..d338066 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -45,6 +45,7 @@ #include "virtime.h" #include "locking/domain_lock.h" #include "rpc/virnetsocket.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -1775,10 +1776,10 @@ static int doNativeMigrate(struct qemud_driver *driver, virReportOOMError(); return -1; } - uribits = xmlParseURI(tmp); + uribits = virParseURI(tmp); VIR_FREE(tmp); } else { - uribits = xmlParseURI(uri); + uribits = virParseURI(uri); } if (!uribits) { qemuReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 2dacb70..96f5f8c 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -47,6 +47,7 @@ #include "command.h" #include "intprops.h" #include "virtypedparam.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_REMOTE @@ -505,7 +506,7 @@ doRemoteOpen (virConnectPtr conn, transport_str[-1] = '\0'; } - name = (char *) xmlSaveUri (&tmpuri); + name = (char *) virSaveURI (&tmpuri); #ifdef HAVE_XMLURI_QUERY_RAW VIR_FREE(tmpuri.query_raw); @@ -719,7 +720,7 @@ doRemoteOpen (virConnectPtr conn, goto failed; VIR_DEBUG("Auto-probed URI is %s", uriret.uri); - conn->uri = xmlParseURI(uriret.uri); + conn->uri = virParseURI(uriret.uri); VIR_FREE(uriret.uri); if (!conn->uri) { virReportOOMError(); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index a4cf945..f1e290a 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -63,6 +63,7 @@ #include "configmake.h" #include "virnetdevtap.h" #include "virnodesuspend.h" +#include "xml.h" #define VIR_FROM_THIS VIR_FROM_UML @@ -1138,7 +1139,7 @@ static virDrvOpenStatus umlOpen(virConnectPtr conn, if (uml_driver == NULL) return VIR_DRV_OPEN_DECLINED; - conn->uri = xmlParseURI(uml_driver->privileged ? + conn->uri = virParseURI(uml_driver->privileged ? "uml:///system" : "uml:///session"); if (!conn->uri) { diff --git a/src/util/xml.c b/src/util/xml.c index b426653..36f3a7f 100644 --- a/src/util/xml.c +++ b/src/util/xml.c @@ -800,6 +800,100 @@ error: goto cleanup; } +/************************************************************************ + * * + * Wrappers around libxml2 URI specific functions * + * * + ************************************************************************/ + +/** + * virParseURI: + * @uri: URI to parse + * + * Wrapper for xmlParseURI + * + * Unfortunately there are few things that should be managed after + * parsing the URI. Fortunately there is only one thing now and its + * removing of square brackets around IPv6 addresses. + * + * @returns the parsed uri object with some fixes + */ +xmlURIPtr +virParseURI(const char *uri) +{ + xmlURIPtr ret = xmlParseURI(uri); + + /* First check: does it even make sense to jump inside */ + if (ret != NULL && + ret->server != NULL && + ret->server[0] == '[') { + size_t length = strlen(ret->server); + + /* We want to modify the server string only if there are + * square brackets on both ends and inside there is IPv6 + * address. Otherwise we could make a mistake by modifying + * something else than IPv6 address. */ + if (ret->server[length - 1] == ']' && strchr(ret->server, ':')) { + memmove(&ret->server[0], &ret->server[1], length - 2); + ret->server[length - 2] = '\0'; + } + /* Even after such modification, it is completely ok to free + * the uri with xmlFreeURI() */ + } + + return ret; +} + +/** + * virSaveURI: + * @uri: URI to save + * + * Wrapper for xmlSaveUri + * + * This function constructs back everything that @ref virParseURI + * changes after parsing + * + * @returns the constructed uri as a string + */ +unsigned char * +virSaveURI(xmlURIPtr uri) +{ + char *tmpserver, *backupserver = uri->server; + unsigned char *ret; + bool fixback = false; + + /* First check: does it make sense to do anything */ + if (uri != NULL && + uri->server != NULL && + strchr(uri->server, ':') != NULL) { + + /* To be sure we have enough space for the brackets, we save + * the old string and then alocate a new one */ + + /* the "+ 2" is room for square brackets and + 1 for '\0' */ + size_t length = strlen(uri->server) + 3; + + if(VIR_ALLOC_N(tmpserver, length) < 0) { + virReportOOMError(); + return NULL; + } + + snprintf(tmpserver, length, "[%s]", uri->server); + + uri->server = tmpserver; + bool fixback = true; + } + + ret = xmlSaveUri(uri); + + /* Put the fixed version back */ + if (fixback) { + uri->server = backupserver; + VIR_FREE(tmpserver); + } + + return ret; +} static int virXMLEmitWarning(int fd, const char *name, diff --git a/src/util/xml.h b/src/util/xml.h index a3750fa..4835900 100644 --- a/src/util/xml.h +++ b/src/util/xml.h @@ -10,6 +10,7 @@ # include <libxml/parser.h> # include <libxml/tree.h> # include <libxml/xpath.h> +# include <libxml/uri.h> int virXPathBoolean(const char *xpath, xmlXPathContextPtr ctxt); @@ -61,6 +62,10 @@ xmlDocPtr virXMLParseHelper(int domcode, const char *url, xmlXPathContextPtr *pctxt); +xmlURIPtr virParseURI(const char *uri); + +unsigned char * virSaveURI(xmlURIPtr uri); + /** * virXMLParse: * @filename: file to parse, or NULL for string parsing diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index b168c7d..11fa995 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -56,6 +56,7 @@ #include "configmake.h" #include "virfile.h" #include "fdstream.h" +#include "xml.h" /* This one changes from version to version. */ #if VBOX_API_VERSION == 2002 @@ -980,7 +981,7 @@ static virDrvOpenStatus vboxOpen(virConnectPtr conn, virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); if (conn->uri == NULL) { - conn->uri = xmlParseURI(uid ? "vbox:///session" : "vbox:///system"); + conn->uri = virParseURI(uid ? "vbox:///session" : "vbox:///system"); if (conn->uri == NULL) { virReportOOMError(); return VIR_DRV_OPEN_ERROR; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 823d5df..92a6e1c 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -33,6 +33,7 @@ #include "logging.h" #include "uuid.h" #include "vmx.h" +#include "xml.h" /* @@ -2608,7 +2609,7 @@ virVMXParseSerial(virVMXContext *ctx, virConfPtr conf, int port, (*def)->target.port = port; (*def)->source.type = VIR_DOMAIN_CHR_TYPE_TCP; - parsedUri = xmlParseURI(fileName); + parsedUri = virParseURI(fileName); if (parsedUri == NULL) { virReportOOMError(); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 635f468..5ba3f1c 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -270,7 +270,7 @@ xenUnifiedOpen (virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) if (!xenUnifiedProbe()) return VIR_DRV_OPEN_DECLINED; - conn->uri = xmlParseURI("xen:///"); + conn->uri = virParseURI("xen:///"); if (!conn->uri) { virReportOOMError(); return VIR_DRV_OPEN_ERROR; diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 5c3838f..e346f5f 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -46,6 +46,7 @@ #include "memory.h" #include "count-one-bits.h" #include "virfile.h" +#include "xml.h" /* required for cpumap_t */ #include <xen/dom0_ops.h> @@ -3224,7 +3225,7 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, * "hostname", "hostname:port" or "xenmigr://hostname[:port]/". */ if (strstr (uri, "//")) { /* Full URI. */ - xmlURIPtr uriptr = xmlParseURI (uri); + xmlURIPtr uriptr = virParseURI (uri); if (!uriptr) { virXendError(VIR_ERR_INVALID_ARG, "%s", _("xenDaemonDomainMigrate: invalid URI")); -- 1.7.3.4