[libvirt] [PATCH 1/2] conf: Check if number of vCPUs fits in the storage variable

Peter Krempa pkrempa at redhat.com
Tue Jan 22 16:47:36 UTC 2013


On 01/22/13 16:07, John Ferlan wrote:
> On 01/22/2013 09:31 AM, Peter Krempa wrote:
>> The count of vCPUs for a domain is extracted as a usingned long variable
>> but is stored in a unsigned short. If the actual number was too large,
>> a faulty number was stored.
>> ---
>>   src/conf/domain_conf.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 0b9ba13..3e95ec9 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -9085,7 +9085,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>>           def->maxvcpus = 1;
>>       } else {
>>           def->maxvcpus = count;
>> -        if (count == 0) {
>> +        if (count == 0 || (unsigned short) count != count) {
>
> maxvcpus is a 'unsigned short' and count is an 'unsigned long', thus if
> def->maxvcpus != count after this point, then we have the overflow,
> right?  Or would the compiler "adjust" that comparison behind our back
> on an if check?

You mean changing the explicit typecast with checking of def->maxvcpus?

This works in gcc too, but I'm afraid I have -O0 as a default for 
libvirt. I'm not sure if a compiler is allowed to optimize such a 
comparison, but the explicit typecast is probably safer and it is more 
noticeable to a possible future reader of that piece of code.

>
>>               virReportError(VIR_ERR_XML_ERROR,
>>                              _("invalid maxvcpus %lu"), count);
>>               goto error;
>> @@ -9101,7 +9101,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>>           def->vcpus = def->maxvcpus;
>>       } else {
>>           def->vcpus = count;
>> -        if (count == 0) {
>> +        if (count == 0 || (unsigned short) count != count) {
>
> Same comment as 'maxvcpus'
>
>>               virReportError(VIR_ERR_XML_ERROR,
>>                              _("invalid current vcpus %lu"), count);
>>               goto error;
>>
>
> ACK - I think what you've done is right, although perhaps someone with a
> bit more knowledge of what the compiler does could pipe in (I'm curious
> too).

For now, I will probably push this as is. We still can fix this in the 
future if there will be a cleaner solution.

Thanks for the review.

Peter
>
> John
>




More information about the libvir-list mailing list