diff mbox

Prevent segmentation fault in case of relative resolve of uri

Message ID 1410248706-9769-1-git-send-email-mrezanin@redhat.com
State New
Headers show

Commit Message

Miroslav Rezanina Sept. 9, 2014, 7:45 a.m. UTC 
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>
---
 util/uri.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Stefan Hajnoczi Sept. 17, 2014, 9:26 a.m. UTC | #1 
On Tue, Sep 09, 2014 at 09:45:06AM +0200, 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>
> ---
>  util/uri.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/util/uri.c b/util/uri.c
> index e348c17..16c01d0 100644
> --- a/util/uri.c
> +++ b/util/uri.c
> @@ -1985,7 +1985,8 @@ uri_resolve_relative (const char *uri, const char * base)
>  	val = g_strdup (uri);
>  	goto done;
>      }
> -    if (!strcmp(bas->path, ref->path)) {
> +    if (bas->path != NULL && ref->path != NULL && 
> +	 !strcmp(bas->path, ref->path)) {
>  	val = g_strdup("");
>  	goto done;
>      }

This patch adds more conditions, what's needed is to audit and clean up this
function.  There is dead code and things could be simplified instead:

    if (!strcmp(bas->path, ref->path)) {
        val = g_strdup("");
        goto done;
    }
    if (bas->path == NULL) {
        val = g_strdup(ref->path);
        goto done;
    }
    if (ref->path == NULL) {
        ref->path = (char *) "/";
        remove_path = 1;
    }
    <---- bas->path cannot be NULL because we took goto done
    <---- ref->path cannot be NULL because we assigned "/"

    /*
     * 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) {  <--- dead code!
        if (ref->path != NULL) {
            uptr = ref->path;
            if (*uptr == '/')
                uptr++;
            /* exception characters from uri_to_string */
            val = uri_string_escape(uptr, "/;&=+$,");
        }
        goto done;
    }
    bptr = bas->path;
    if (ref->path == NULL) {  <--- dead code!

Also, perhaps the strcmp() you touched should really be moved below the NULL
checks.
Markus Armbruster Sept. 18, 2014, 2:54 p.m. UTC | #2 
Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Tue, Sep 09, 2014 at 09:45:06AM +0200, 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>
>> ---
>>  util/uri.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/util/uri.c b/util/uri.c
>> index e348c17..16c01d0 100644
>> --- a/util/uri.c
>> +++ b/util/uri.c
>> @@ -1985,7 +1985,8 @@ uri_resolve_relative (const char *uri, const char * base)
>>  	val = g_strdup (uri);
>>  	goto done;
>>      }
>> -    if (!strcmp(bas->path, ref->path)) {
>> +    if (bas->path != NULL && ref->path != NULL && 
>> +	 !strcmp(bas->path, ref->path)) {
>>  	val = g_strdup("");
>>  	goto done;
>>      }
>
> This patch adds more conditions, what's needed is to audit and clean up this
> function.  There is dead code and things could be simplified instead:
>
>     if (!strcmp(bas->path, ref->path)) {
>         val = g_strdup("");
>         goto done;
>     }
>     if (bas->path == NULL) {
>         val = g_strdup(ref->path);
>         goto done;
>     }
>     if (ref->path == NULL) {
>         ref->path = (char *) "/";
>         remove_path = 1;
>     }
>     <---- bas->path cannot be NULL because we took goto done
>     <---- ref->path cannot be NULL because we assigned "/"
>
>     /*
>      * 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) {  <--- dead code!
>         if (ref->path != NULL) {
>             uptr = ref->path;
>             if (*uptr == '/')
>                 uptr++;
>             /* exception characters from uri_to_string */
>             val = uri_string_escape(uptr, "/;&=+$,");
>         }
>         goto done;
>     }
>     bptr = bas->path;
>     if (ref->path == NULL) {  <--- dead code!
>
> Also, perhaps the strcmp() you touched should really be moved below the NULL
> checks.

If you simplify this function, please get rid of the silly conditionals
around g_strdup().  See commits 80e0090 c14e984 c64f50d for examples.
Markus Armbruster Sept. 18, 2014, 2:57 p.m. UTC | #3 
Markus Armbruster <armbru@redhat.com> writes:

> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
>> On Tue, Sep 09, 2014 at 09:45:06AM +0200, 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>
>>> ---
>>>  util/uri.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/util/uri.c b/util/uri.c
>>> index e348c17..16c01d0 100644
>>> --- a/util/uri.c
>>> +++ b/util/uri.c
>>> @@ -1985,7 +1985,8 @@ uri_resolve_relative (const char *uri, const char * base)
>>>  	val = g_strdup (uri);
>>>  	goto done;
>>>      }
>>> -    if (!strcmp(bas->path, ref->path)) {
>>> +    if (bas->path != NULL && ref->path != NULL && 
>>> +	 !strcmp(bas->path, ref->path)) {
>>>  	val = g_strdup("");
>>>  	goto done;
>>>      }
>>
>> This patch adds more conditions, what's needed is to audit and clean up this
>> function.  There is dead code and things could be simplified instead:
>>
>>     if (!strcmp(bas->path, ref->path)) {
>>         val = g_strdup("");
>>         goto done;
>>     }
>>     if (bas->path == NULL) {
>>         val = g_strdup(ref->path);
>>         goto done;
>>     }
>>     if (ref->path == NULL) {
>>         ref->path = (char *) "/";
>>         remove_path = 1;
>>     }
>>     <---- bas->path cannot be NULL because we took goto done
>>     <---- ref->path cannot be NULL because we assigned "/"
>>
>>     /*
>>      * 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) {  <--- dead code!
>>         if (ref->path != NULL) {
>>             uptr = ref->path;
>>             if (*uptr == '/')
>>                 uptr++;
>>             /* exception characters from uri_to_string */
>>             val = uri_string_escape(uptr, "/;&=+$,");
>>         }
>>         goto done;
>>     }
>>     bptr = bas->path;
>>     if (ref->path == NULL) {  <--- dead code!
>>
>> Also, perhaps the strcmp() you touched should really be moved below the NULL
>> checks.
>
> If you simplify this function, please get rid of the silly conditionals
> around g_strdup().  See commits 80e0090 c14e984 c64f50d for examples.

Likewise: g_free().  See commits 9e28840 64641d8 f7047c2 fb7da62 fec0da9
ec15993.
diff mbox

Patch

diff --git a/util/uri.c b/util/uri.c
index e348c17..16c01d0 100644
--- a/util/uri.c
+++ b/util/uri.c
@@ -1985,7 +1985,8 @@  uri_resolve_relative (const char *uri, const char * base)
 	val = g_strdup (uri);
 	goto done;
     }
-    if (!strcmp(bas->path, ref->path)) {
+    if (bas->path != NULL && ref->path != NULL && 
+	 !strcmp(bas->path, ref->path)) {
 	val = g_strdup("");
 	goto done;
     }