On 10/31/2013 05:23 PM, Jim Fehlig wrote: > Jeremy Fitzhardinge wrote: >> On 10/24/2013 02:52 AM, Martin Kletzander wrote: >>> On Wed, Oct 23, 2013 at 10:46:14AM -0700, Jeremy Fitzhardinge wrote: >>>> Hi all, >>>> >>>> I posted this bug (https://bugzilla.redhat.com/show_bug.cgi?id=1013045) >>>> to the Redhat Bugzilla a while ago, and the only response has been to >>>> post a note to this list about the bug. >>>> >>>> Summary below, but it looks like a pretty clear use-after-free or >>>> something. The full details are attached to the bug report. >>>> >>> From the looks of the BZ, I think the probnlem found by valgrind (not >>> the one in libxl) is a different than the one which causes the >>> 'invalid free()', but anyway, I posted a patch [1] which might help >>> (read: fixes a problem found out thanks to the valgrind output), but I >>> have no way to test it. If you do, I would appreciate you trying >>> whether the issue gets fixed for you with that patch. >> I reverted your change then applied the following, which looks like it >> fixes the problem. >> >> Thanks, >> >> J >> >> >> commit 65d342a6df5e8020b682a6085aa7aced7215e93b >> Author: Jeremy Fitzhardinge <jeremy@xxxxxxxx> >> Date: Wed Oct 30 10:36:37 2013 -0700 >> >> libxl: fix dubious cpumask handling in libxlDomainSetVcpuAffinities >> >> Rather than casting the virBitmap pointer to uint8_t* and then using >> the structure contents as a byte array, use the virBitmap API to >> determine >> the bitmap size and test each bit. > Yikes... > >> Signed-off-by: Jeremy Fitzhardinge <jeremy@xxxxxxxx> >> >> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c >> index e2a6d44..ab509a6 100644 >> --- a/src/libxl/libxl_driver.c >> +++ b/src/libxl/libxl_driver.c >> @@ -448,7 +448,7 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr >> driver, virDomainObjPtr vm) >> libxlDomainObjPrivatePtr priv = vm->privateData; >> virDomainDefPtr def = vm->def; >> libxl_bitmap map; >> - uint8_t *cpumask = NULL; >> + virBitmapPtr cpumask = NULL; >> uint8_t *cpumap = NULL; >> virNodeInfo nodeinfo; >> size_t cpumaplen; >> @@ -468,10 +468,16 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr >> driver, virDomainObjPtr vm) >> if (VIR_ALLOC_N(cpumap, cpumaplen) < 0) >> goto cleanup; >> >> - cpumask = (uint8_t*) def->cputune.vcpupin[vcpu]->cpumask; >> + cpumask = def->cputune.vcpupin[vcpu]->cpumask; >> >> - for (i = 0; i < VIR_DOMAIN_CPUMASK_LEN; ++i) { >> - if (cpumask[i]) >> + for (i = 0; i < virBitmapSize(cpumask); ++i) { >> + bool bit; >> + if (virBitmapGetBit(cpumask, i, &bit) < 0) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, >> + _("Failed to get cpumask bit '%zd' with >> libxenlight"), i); > I don't think that is the most informative error message, but can't > really suggest anything better since I'm having a problem understanding > how virtBitmapGetBit() could fail. The virDomainObjPtr is locked, so the > cpumask for the vcpu should not change. We just asked for the size of > the map, thus shouldn't be asking for a bit outside that range, right? > > I suppose if failure was somehow possible, it would indicate > misconfiguration. E.g. an invalid cpumask for the vcpu. If so, something > like "Failed to decode cpumask for vcpu '%zd'" is more helpful It returns an error, so out of an abundance of caution I test for the error. But no, I can't see how it could happen. J > > Regards, > Jim > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list