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@xxxxxxxxxx>
>>> ---
>>> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list