[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] Fixed URI parsing



On 02/24/2012 06:30 AM, Martin Kletzander wrote:
> 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.
> 

> + */
> +unsigned char *
> +virSaveURI(xmlURIPtr uri)
> +{
> +    char *tmpserver, *backupserver = uri->server;
> +    unsigned char *ret;
> +    bool fixback = false;

Instead of using bool fixback, I'd set tmpserver as NULL here...

> +
> +    /* 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 */

s/alocate/allocate/ - but see below, as I don't think you need this comment.

> +
> +         /* 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();

No need to raise OOM error here - all callers of xmlSaveUri were already
raising their own OOM after a NULL return.

> +            return NULL;
> +        }
> +
> +        snprintf(tmpserver, length, "[%s]", uri->server);

I'd just use virAsnprintf(&tmpserver, "[%s]", uri->server); none of this
need to call strlen, VIR_ALLOC_N, or snprintf.

> +
> +        uri->server = tmpserver;
> +        bool fixback = true;
> +    }
> +
> +    ret = xmlSaveUri(uri);
> +
> +    /* Put the fixed version back */
> +    if (fixback) {

...and check 'if (tmpserver)' here (that is, fixback is redundant with
whether tmpserver was assigned at this point).

> +        uri->server = backupserver;
> +        VIR_FREE(tmpserver);
> +    }
> +
> +    return ret;
> +}
> 

I'm also a bit worried that we might regress if we don't add a syntax
check; can you look at adding a rule to cfg.mk that poisons xmlParseURI
and xmlSaveUri (wow, libxml2 really does have the awful URI vs. Uri in
its naming conventions), then exempt util/uri.c from the check as the
only file allowed to use them.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]