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(a)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