[libvirt] [PATCH 1/2] qemuBuildMemoryBackendStr: Don't crash if no hugetlbfs is mounted

[Michal Privoznik]

On 19.09.2016 08:32, Peter Krempa wrote:
> On Mon, Sep 19, 2016 at 07:55:47 +0200, Michal Privoznik wrote:
>> When trying to migrate a huge page enabled guest, I've noticed
>> the following crash. Apparently, if no specific hugepages are
>> requested:
>>
>>   <memoryBacking>
>>     <hugepages/>
>>   </memoryBacking>
>>
>> and there are no hugepages configured on the destination, we try
>> to dereference a NULL pointer.
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x00007fcc907fb20e in qemuGetHugepagePath (hugepage=0x0) at qemu/qemu_conf.c:1447
>> 1447        if (virAsprintf(&ret, "%s/libvirt/qemu", hugepage->mnt_dir) < 0)
>> (gdb) bt
>> #0  0x00007fcc907fb20e in qemuGetHugepagePath (hugepage=0x0) at qemu/qemu_conf.c:1447
>> #1  0x00007fcc907fb2f5 in qemuGetDefaultHugepath (hugetlbfs=0x0, nhugetlbfs=0) at qemu/qemu_conf.c:1466
>> #2  0x00007fcc907b4afa in qemuBuildMemoryBackendStr (size=4194304, pagesize=0, guestNode=0, userNodeset=0x0, autoNodeset=0x0, def=0x7fcc70019070, qemuCaps=0x7fcc70004000, cfg=0x7fcc5c011800, backendType=0x7fcc95087228, backendProps=0x7fcc95087218,
>>     force=false) at qemu/qemu_command.c:3297
>> #3  0x00007fcc907b4f91 in qemuBuildMemoryCellBackendStr (def=0x7fcc70019070, qemuCaps=0x7fcc70004000, cfg=0x7fcc5c011800, cell=0, auto_nodeset=0x0, backendStr=0x7fcc70020360) at qemu/qemu_command.c:3413
>> #4  0x00007fcc907c0406 in qemuBuildNumaArgStr (cfg=0x7fcc5c011800, def=0x7fcc70019070, cmd=0x7fcc700040c0, qemuCaps=0x7fcc70004000, auto_nodeset=0x0) at qemu/qemu_command.c:7470
>> #5  0x00007fcc907c5fdf in qemuBuildCommandLine (driver=0x7fcc5c07b8a0, logManager=0x7fcc70003c00, def=0x7fcc70019070, monitor_chr=0x7fcc70004bb0, monitor_json=true, qemuCaps=0x7fcc70004000, migrateURI=0x7fcc700199c0 "defer", snapshot=0x0,
>>     vmop=VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START, standalone=false, enableFips=false, nodeset=0x0, nnicindexes=0x7fcc95087498, nicindexes=0x7fcc950874a0, domainLibDir=0x7fcc700047c0 "/var/lib/libvirt/qemu/domain-1-fedora") at qemu/qemu_command.c:9547
> 
> I think you can trim the backtrace here.

Okay,

>> ---
>>  src/qemu/qemu_command.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 038c38f..0bafc3f 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -3294,6 +3294,12 @@ qemuBuildMemoryBackendStr(unsigned long long size,
>>              if (!(mem_path = qemuGetHugepagePath(&cfg->hugetlbfs[i])))
>>                  goto cleanup;
>>          } else {
>> +            if (!cfg->nhugetlbfs) {
>> +                virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                               "%s", _("hugetlbfs filesystem is not mounted "
>> +                                       "or disabled by administrator config"));
>> +                goto cleanup;
>> +            }
>>              if (!(mem_path = qemuGetDefaultHugepath(cfg->hugetlbfs,
>>                                                      cfg->nhugetlbfs)))
> 
> I think the bug is in qemuGetDefaultHugepath function rather than the
> caller:
> 
> char *
> qemuGetDefaultHugepath(virHugeTLBFSPtr hugetlbfs,
>                        size_t nhugetlbfs)
> {
>     size_t i;
> 
>     for (i = 0; i < nhugetlbfs; i++)
>         if (hugetlbfs[i].deflt)
>             break;
> 
>     if (i == nhugetlbfs)
>         i = 0;
> 
>     return qemuGetHugepagePath(&hugetlbfs[i]);
> 
> ^^ it dereferences the first member of the first argument even if you
> explicitly specify that the array has 0 elements.

That i = 0 assignment does not state array size, it just picks the first
element in the array if no explicitly set default huge page size is found.

> 
> }
> 
> If you want to go with the fix as in this patch then you'll need to add
> a comment to qemuGetDefaultHugepath that the callers should make sure
> that it's called with at least one hugepage entry. Or fix
> qemuGetDefaultHugepath rathr than working it around in the caller.

Well, after my 2nd patch, there's no other caller than a fixed one.
And I think qemuGetDefaultHugepath is really designed to be just a
simple function to fetch caller the path to huge pages. But okay, I can
discard 2/2 and basically move out its internals to various other
functions (like qemuGetDefaultHugepath).

Michal