[libvirt] [PATCH] cpu: fix possible crash in getModels

[Pavel Hrdina]

On 12/03/2014 07:06 PM, Peter Krempa wrote:
> On 12/03/14 19:02, Pavel Hrdina wrote:
>> Commit 86a15a25 introduced a new cpu driver API 'getModels'. Public API
>> allow you to pass NULL for models to get only number of existing models.
>> However the new code will crash with segfault so we have to count with
>> the possibility that the user wants only the number.
>>
>> There is also difference in order of the models gathered by this new API
>> as the old approach was inserting the elements to the end of the array
>> so we should use 'VIR_APPEND_ELEMENT'.
>>
>> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>> ---
>>   src/cpu/cpu_powerpc.c | 15 ++++++++++-----
>>   src/cpu/cpu_x86.c     | 15 ++++++++++-----
>>   2 files changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c
>> index 871401b..86f80b3 100644
>> --- a/src/cpu/cpu_powerpc.c
>> +++ b/src/cpu/cpu_powerpc.c
>> @@ -666,11 +666,15 @@ ppcGetModels(char ***models)
>>
>>       model = map->models;
>>       while (model != NULL) {
>> -        if (VIR_STRDUP(name, model->name) < 0)
>> -            goto error;
>> +        if (models) {
>> +            if (VIR_STRDUP(name, model->name) < 0)
>> +                goto error;
>>
>> -        if (VIR_INSERT_ELEMENT(*models, 0, nmodels, name) < 0)
>> -            goto error;
>> +            if (VIR_APPEND_ELEMENT(*models, nmodels, name) < 0)
>> +                goto error;
>> +        } else {
>> +            nmodels++;
>> +        }
>>
>>           model = model->next;
>>       }
>> @@ -681,7 +685,8 @@ ppcGetModels(char ***models)
>>       return nmodels;
>>
>>    error:
>> -    virStringFreeList(*models);
>> +    if (models)
>> +        virStringFreeList(*models);
>
> if (models) {
> 	virStringFreeList(*models);
> 	*models = NULL;
> }
>
> To remove the possibly invalid pointer.
>
>>       nmodels = -1;
>>       goto cleanup;
>>   }
>> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
>> index f6dcba4..dfbc16c 100644
>> --- a/src/cpu/cpu_x86.c
>> +++ b/src/cpu/cpu_x86.c
>> @@ -2176,11 +2176,15 @@ x86GetModels(char ***models)
>>
>>       model = map->models;
>>       while (model != NULL) {
>> -        if (VIR_STRDUP(name, model->name) < 0)
>> -            goto error;
>> +        if (models) {
>> +            if (VIR_STRDUP(name, model->name) < 0)
>> +                goto error;
>>
>> -        if (VIR_INSERT_ELEMENT(*models, 0, nmodels, name) < 0)
>> -            goto error;
>> +            if (VIR_APPEND_ELEMENT(*models, nmodels, name) < 0)
>> +                goto error;
>> +        } else {
>> +            nmodels++;
>> +        }
>>
>>           model = model->next;
>>       }
>> @@ -2188,7 +2192,8 @@ x86GetModels(char ***models)
>>       return nmodels;
>>
>>    error:
>> -    virStringFreeList(*models);
>> +    if (models)
>> +        virStringFreeList(*models);
>
> Same as above.
>
>>       return -1;
>>   }
>>
>>
>
> ACK,
>
> Peter
>

Thanks, I'll push it with that change after my coverity build ends.

Pavel