[libvirt] [PATCH] util: fix libvirtd crash caused by virStorageNetHostTransportTypeFromString
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@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
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.
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@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
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@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
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@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
participants (3)
-
Eric Blake -
Peter Krempa -
Shanzhi Yu