Re: [Qemu-devel] [PATCHv2] Prevent segmentation fault in case of relative resolve of uri

[Paolo Bonzini <pbonzini@redhat.com>]

On 09/02/2015 11:47, mrezanin@redhat.com wrote:
> From: Miroslav Rezanina <mrezanin@redhat.com>
> 
> It was possible to call strcmp with NULL argument, that can cause
> segmentation fault. Properly checking parameters to prevent this
> situation.
> 
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> ---
> v2:
>  - instead of adding NULL checks to strcmp call refactor whole
>    NULL checking path. This will remove dead code and make whole checking
>    easier to understand.
> 
>    Relative path generation part is not touched as I'm not fully sure
>    of correct behavior and purpose of this patch is to prevent segmentation
>    fault.
> ---
>  util/uri.c | 55 +++++++++++++++++++++++++------------------------------
>  1 file changed, 25 insertions(+), 30 deletions(-)
> 
> diff --git a/util/uri.c b/util/uri.c
> index 918d235..23dbaca 100644
> --- a/util/uri.c
> +++ b/util/uri.c
> @@ -1964,44 +1964,39 @@ uri_resolve_relative (const char *uri, const char * base)
>       * If the scheme / server on the URI differs from the base,
>       * just return the URI
>       */
> -    if ((ref->scheme != NULL) &&
> -	((bas->scheme == NULL) ||
> -	 (strcmp (bas->scheme, ref->scheme)) ||
> -	 (strcmp (bas->server, ref->server)))) {
> -	val = g_strdup (uri);
> -	goto done;
> +
> +    if ((ref->scheme != NULL) && 
> +        ((bas->scheme == NULL) || (strcmp (bas->scheme, ref->scheme)))) {
> +        val = g_strdup(uri);
> +        goto done;
>      }
> -    if (!strcmp(bas->path, ref->path)) {
> -	val = g_strdup("");
> -	goto done;
> -    }
> -    if (bas->path == NULL) {
> -	val = g_strdup(ref->path);
> -	goto done;
> +    if ((ref->server != NULL) &&
> +        ((bas->server == NULL) || (strcmp (bas->server, ref->server)))) {
> +        val = g_strdup(uri);
> +        goto done;
>      }
> +
>      if (ref->path == NULL) {
>          ref->path = (char *) "/";
> -	remove_path = 1;
> +        remove_path = 1;
>      }
>  
> -    /*
> -     * At this point (at last!) we can compare the two paths
> -     *
> -     * First we take care of the special case where either of the
> -     * two path components may be missing (bug 316224)
> -     */
>      if (bas->path == NULL) {
> -	if (ref->path != NULL) {
> -	    uptr = ref->path;
> -	    if (*uptr == '/')
> -		uptr++;
> -	    /* exception characters from uri_to_string */
> -	    val = uri_string_escape(uptr, "/;&=+$,");
> -	}
> -	goto done;
> +        uptr = ref->path;
> +        if (*uptr == '/')
> +            uptr++;
> +        /* exception characters from uri_to_string */
> +        val = uri_string_escape(uptr, "/;&=+$,");
> +        goto done;
>      }
> +
> +    if (!strcmp(bas->path, ref->path)) {
> +        val = g_strdup("");
> +        goto done;
> +    }
> +
>      bptr = bas->path;
> -    if (ref->path == NULL) {
> +    if (remove_path == 1) {
>  	for (ix = 0; bptr[ix] != 0; ix++) {
>  	    if (bptr[ix] == '/')
>  		nbslash++;
> @@ -2010,7 +2005,7 @@ uri_resolve_relative (const char *uri, const char * base)
>  	len = 1;	/* this is for a string terminator only */
>      } else {
>      /*
> -     * Next we compare the two strings and find where they first differ
> +     * We compare the two strings and find where they first differ
>       */
>  	if ((ref->path[pos] == '.') && (ref->path[pos+1] == '/'))
>              pos += 2;
> 

It's the third time a fix for this is submitted. :)

[PATCH 3/3] util/uri: URI member path can be null, compare more carfully
(by Markus)

[PATCH 3/7] uri: avoid NULL arguments to strcmp
(by me).

Markus's patch was accepted.  Further cleanups on the code on top of his
patch are welcome, though.

The logic for "compare two possibly-NULL strings" could be replaced by
g_strcmp0, but that's only provided by glib versions 2.16 or newer,
while we support 2.12.

Paolo