[libvirt] RHBZ 1013045: Crash on xen domain startup

[Jeremy Fitzhardinge]

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 at goop.org>
>> 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 at goop.org>
>>
>> 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
>