[libvirt] [PATCH] util: fix libvirtd crash caused by virStorageNetHostTransportTypeFromString

[Peter Krempa]

On 10/27/14 07:40, Shanzhi Yu wrote:
> 
> On 10/25/2014 03:12 AM, Eric Blake wrote:
>> On 10/24/2014 01:01 PM, Shanzhi Yu wrote:
>>> When split uri->scheme into two strings with "+", the second one will be
>> s/split/splitting/
>>
>>> "rdma://server/..", pass it to virStorageNetHostTransportTypeFromString
>>> will lead libvirtd crash. So a second virStringSplit call is needed.
>> Can you show the FULL string that is being passed into this function,
>> and not just the string after the first split on '+'?  That is, showing
>> an easy formula of how to reproduce the bug makes it easier to know if
>> the solution is right.
> 
> Seem the solution is not right. I misunderstand uri->scheme as
> "gluster+tcp://NetHostIP/path/to/volume"
> The scheme should be "gluster+tcp" or "gluster+rdma" ?
> ..
> 
>     if (!(uri = virURIParse(path))) {
>         virReportError(VIR_ERR_INTERNAL_ERROR,
>                        _("failed to parse backing file location '%s'"),
>                        path);
>         goto cleanup;
>     }
> 
>     if (!(scheme = virStringSplit(uri->scheme, "+", 2)))
>         goto cleanup;
> ..
> 
> But it is easy to reproduce this crash
> 
> Steps:
> 
> 1) Create a volume with "gluster+tcp" or "gluster+rmda" as backend
> # qemu-img create -f qcow2 /var/lib/libvirt/images/tcp.img -b
> gluster+tcp://10.66.6.111/gluster-vol1/rhel65.img -o backing_fmt=raw 1G
> Formatting '/var/lib/libvirt/images/tcp.img', fmt=qcow2 size=1073741824
> backing_file='gluster+tcp://10.66.6.111/gluster-vol1/rhel65.img'
> backing_fmt='raw' encryption=off cluster_size=65536 lazy_refcounts=off
> 
> 2) Refresh pool default (target pach /var/lib/libvirt/images)
> # virsh pool-refresh default
> 
> Libvirtd will crash
> 
> ..
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7f5df5e4d700 (LWP 23916)]
> virStorageSourceParseBackingURI (path=<optimized out>,
> src=0x7f5de0009130) at util/virstoragefile.c:2126
> 2126            (src->hosts->transport =
> virStorageNetHostTransportTypeFromString(scheme[1])) < 0) {
> (gdb)
> 
> ..
> 
> 
>>
>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1156288
>> You have to assume that not everyone will click through this link.
>>
>>> Signed-off-by: Shanzhi Yu <shyu at redhat.com>
>>> ---
>>>   src/util/virstoragefile.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>>> index 960aa23..795c188 100644
>>> --- a/src/util/virstoragefile.c
>>> +++ b/src/util/virstoragefile.c
>>> @@ -2144,6 +2144,9 @@
>>> virStorageSourceParseBackingURI(virStorageSourcePtr src,
>>>           goto cleanup;
>>>       }
>>>   +    if (!(scheme = virStringSplit(scheme[1], ":", 2)))
>> Ouch. Memory leak.  You are overwriting the contents of malloc'd scheme
>> with a new pointer.  You'll need to send a v2.
> 
> As talk before, seem the solution is wrong. But I do escape the crash
> with below patch
> 

The patch below avoids the crash as it avoids parsing the scheme at all.
The problem is different though:

> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 960aa23..a8373af 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -2124,6 +2124,7 @@
> virStorageSourceParseBackingURI(virStorageSourcePtr src,
>  {
>      virURIPtr uri = NULL;
>      char **scheme = NULL;
> +    char **scheme1 = NULL;
>      int ret = -1;
> 
>      if (!(uri = virURIParse(path))) {
> @@ -2144,11 +2145,14 @@
> virStorageSourceParseBackingURI(virStorageSourcePtr src,
>          goto cleanup;
>      }
> 
> -    if (scheme[1] &&
> -        (src->hosts->transport =
> virStorageNetHostTransportTypeFromString(scheme[1])) <
> +    if (scheme[1] && !(scheme1 = virStringSplit(scheme[1], ":", 2)))
> +        goto cleanup;

The scheme doesn't contain the colon thus scheme1 will always have only
one element

> +
> +    if (scheme1[1] &&

and thus never trigger this condition.

> +        (src->hosts->transport =

The real problem is here though. src->hosts is not allocated and thus
dereferencing it crashes libvirt. The code that allocates it is a bit
below and needs to be moved upwards.

I'll be sending a patch shortly. Please make sure that you are fixing
the right problem next time.

> virStorageNetHostTransportTypeFromString(scheme1[1]))
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("invalid protocol transport type '%s'"),
> -                       scheme[1]);
> +                       scheme1[1]);
>          goto cleanup;
>      }
> 
> @@ -2201,6 +2205,8 @@
> virStorageSourceParseBackingURI(virStorageSourcePtr src,
>   cleanup:
>      virURIFree(uri);
>      virStringFreeList(scheme);
> +    if (!scheme1)
> +        virStringFreeList(scheme1);
>      return ret;
>  }
> 
> 
> 
>>> +        goto cleanup;
>>> +
>>>       if (scheme[1] &&
>>>           (src->hosts->transport =
>>> virStorageNetHostTransportTypeFromString(scheme[1])) < 0) {
>>>           virReportError(VIR_ERR_INTERNAL_ERROR,
>>>
> 

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141029/4401f6f5/attachment-0001.sig>