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 <s...@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
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list