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

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> Stefan Hajnoczi <address@hidden> writes:
>
>> On Tue, Sep 09, 2014 at 09:45:06AM +0200, address@hidden wrote:
>>> From: Miroslav Rezanina <address@hidden>
>>> 
>>> 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 <address@hidden>
>>> ---
>>>  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.