Re: [libvirt] [PATCH] libxl: Fix possible invalid read

On Thu, Oct 24, 2013 at 10:50:02AM +0100, Martin Kletzander wrote: > According to the following valgrind output, there seems to be a > invalid limit for the iterator (captured on Fedora 19): > > ==3945== Invalid read of size 1 > ==3945== at 0x1E1FA410: libxlVmStart (libxl_driver.c:475) > ==3945== by 0x1E1FAD9A: libxlDomainCreateWithFlags (libxl_driver.c:2633) > ==3945== by 0x5187D46: virDomainCreate (libvirt.c:9439) > ==3945== by 0x13BAA6: remoteDispatchDomainCreateHelper (remote_dispatch.h:2910) > ==3945== by 0x51DE5B9: virNetServerProgramDispatch (virnetserverprogram.c:435) > ==3945== by 0x51D93E7: virNetServerHandleJob (virnetserver.c:165) > ==3945== by 0x50F5BF4: virThreadPoolWorker (virthreadpool.c:144) > ==3945== by 0x50F5670: virThreadHelper (virthreadpthread.c:161) > ==3945== by 0x8046C52: start_thread (pthread_create.c:308) > ==3945== by 0x8758E1C: clone (clone.S:113) > ==3945== Address 0x23424d81 is 0 bytes after a block of size 1 alloc'd > ==3945== at 0x4A08121: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) > ==3945== by 0x50B1F8C: virAllocN (viralloc.c:189) > ==3945== by 0x1E1FA3CA: libxlVmStart (libxl_driver.c:468) > ==3945== by 0x1E1FAD9A: libxlDomainCreateWithFlags (libxl_driver.c:2633) > ==3945== by 0x5187D46: virDomainCreate (libvirt.c:9439) > ==3945== by 0x13BAA6: remoteDispatchDomainCreateHelper (remote_dispatch.h:2910) > ==3945== by 0x51DE5B9: virNetServerProgramDispatch (virnetserverprogram.c:435) > ==3945== by 0x51D93E7: virNetServerHandleJob (virnetserver.c:165) > ==3945== by 0x50F5BF4: virThreadPoolWorker (virthreadpool.c:144) > ==3945== by 0x50F5670: virThreadHelper (virthreadpthread.c:161) > ==3945== by 0x8046C52: start_thread (pthread_create.c:308) > ==3945== by 0x8758E1C: clone (clone.S:113) > ==3945== > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1013045 > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index e2a6d44..4928695 100644 > --- a/src/libxl/libxl_driver.c > +++ b/src/libxl/libxl_driver.c > @@ -454,6 +454,7 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm) > size_t cpumaplen; > int vcpu; > size_t i; > + size_t limit; > int ret = -1; > > if (libxlDoNodeGetInfo(driver, &nodeinfo) < 0) > @@ -470,7 +471,8 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, virDomainObjPtr vm) > > cpumask = (uint8_t*) def->cputune.vcpupin[vcpu]->cpumask; > > - for (i = 0; i < VIR_DOMAIN_CPUMASK_LEN; ++i) { > + limit = MIN(VIR_DOMAIN_CPUMASK_LEN, cpumaplen); > + for (i = 0; i < limit; ++i) { > if (cpumask[i]) > VIR_USE_CPU(cpumap, i); > } Hmm, what is strange about that bug is that the crash is in free() wrt memory corruption, but valgrind only reports bogus reads. Such a crash in free() usually implies a bogus write. The VIR_USE_CPU macro does do such a write though, so the patch looks sane - valgrind must have just not seen it

ACK

Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|