Re: [libvirt] [PATCH] util: fix libvirtd crash caused by virStorageNetHostTransportTypeFromString
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