[libvirt] [PATCH] util: fix libvirtd crash caused by virStorageNetHostTransportTypeFromString

[Shanzhi Yu]

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 at 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