[libvirt] RHBZ 1013045: Crash on xen domain startup

[Jim Fehlig]

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

Regards,
Jim