[libvirt] [PATCH] Fixed URI parsing

Laine Stump laine at laine.org
Thu Feb 9 15:38:03 UTC 2012


On 02/09/2012 09:43 AM, Martin Kletzander wrote:
> Function virParseURI was added. This function is wrapper around
> xmlParseURI. In this wrapper we are fixing what doesn't seems to be
> fixed in libxml2 in the near future. The wrapper is written in such
> way that if anything gets fixed in libxml2, it'll still work.
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).

>
> 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
> ---
>   src/esx/esx_driver.c       |    3 ++-
>   src/libvirt.c              |   10 ++++++----
>   src/libvirt_private.syms   |    1 +
>   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 |    3 ++-
>   src/uml/uml_driver.c       |    3 ++-
>   src/util/xml.c             |   37 +++++++++++++++++++++++++++++++++++++
>   src/util/xml.h             |    3 +++
>   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, 70 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 8035add..eeca680 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
> @@ -1117,8 +1118,9 @@ do_open (const char *name,
>           if (STRCASEEQ(name, "xen"))
>               name = "xen:///";
>
> -        /* Convert xen:// ->  xen:/// because xmlParseURI cannot parse the
> -         * former.  This allows URIs such as xen://localhost to work.
> +        /* Convert xen:// ->  xen:/// because xmlParseURI (and thus virParseURI
> +         * as well) cannot parse the former.  This allows URIs such as
> +         * xen://localhost to work.
>            */
>           if (STREQ (name, "xen://"))
>               name = "xen:///";
> @@ -1127,7 +1129,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"),
> @@ -4964,7 +4966,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 a5f9433..7975f94 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1425,6 +1425,7 @@ virTypedParameterAssign;
>
>
>   # xml.h
> +virParseURI;
>   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 b712da4..9c3c005 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -59,6 +59,7 @@
>   #include "virnodesuspend.h"
>   #include "virtime.h"
>   #include "virtypedparam.h"
> +#include "xml.h"
>
>   #define VIR_FROM_THIS VIR_FROM_LXC
>
> @@ -138,7 +139,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 45c4100..5ff1313 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 12cfbde..35a2ebc 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
> @@ -1773,10 +1774,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 e068126..116cf12 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
>
> @@ -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..982ce9a 100644
> --- a/src/util/xml.c
> +++ b/src/util/xml.c
> @@ -800,6 +800,43 @@ error:
>       goto cleanup;
>   }
>
> +/**
> + * virParseURI:
> + * @uri: URI to parse
> + *
> + * Wrapper for xmlParseURI
> + *
> + * Unfortunately there are few things that should be managed after
> + * parsing the URI. First example is removing the 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 last = strlen(ret->server) - 1; /* Index of last char */
> +
> +        /* 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[last] == ']'&&  strchr(ret->server, ':')) {

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).

> +            for (size_t i = 0; i<  last; i++)
> +                ret->server[i] = ret->server[i + 1];

(this actually copies one character too many, but that's harmless). Just 
replace this with:

               memmove(ret->server, ret->server+1, last-1)

> +
> +            ret->server[last - 1] = '\0';
> +        }
> +        /* Even after such modification, it is completely ok to free
> +         * the uri with xmlFreeURI() */
> +    }
> +
> +    return ret;
> +}
>
>   static int virXMLEmitWarning(int fd,
>                                const char *name,
> diff --git a/src/util/xml.h b/src/util/xml.h
> index a3750fa..c0fbbca 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,8 @@ xmlDocPtr      virXMLParseHelper(int domcode,
>                                    const char *url,
>                                    xmlXPathContextPtr *pctxt);
>
> +xmlURIPtr            virParseURI(const char *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 1fdbd50..866b150 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"
>
>   /*
>
> @@ -2610,7 +2611,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 94cafde..c101e11 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
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>




More information about the libvir-list mailing list