2:01 p.m.
When split uri->scheme into two strings with "+", the second one will be
"rdma://server/..", pass it to virStorageNetHostTransportTypeFromString
will lead libvirtd crash. So a second virStringSplit call is needed.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1156288
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)))
+ goto cleanup;
+
if (scheme[1] &&
(src->hosts->transport =
virStorageNetHostTransportTypeFromString(scheme[1])) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
--
1.9.3
2:12 p.m.
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.
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.
+ goto cleanup;
+
if (scheme[1] &&
(src->hosts->transport =
virStorageNetHostTransportTypeFromString(scheme[1])) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:40 a.m.
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
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;
+
+ if (scheme1[1] &&
+ (src->hosts->transport =
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,
>
--
Regards
shyu
4:50 a.m.
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
3677
days inactive
3682
days old
3 comments
3 participants
participants (3)
-
Eric Blake -
Peter Krempa -
Shanzhi Yu