[libvirt] Re: [PATCH] Remove unsafe strncpy from esx_vmx.c

[Chris Lalancette]

Matthias Bolte wrote:
>> diff --git a/src/esx/esx_vmx.c b/src/esx/esx_vmx.c
>> index af5234e..91a86e2 100644
>> --- a/src/esx/esx_vmx.c
>> +++ b/src/esx/esx_vmx.c
>> @@ -397,10 +397,7 @@ def->parallels[0]...
>>
>>  #define ESX_BUILD_VMX_NAME(_suffix)                                           \
>>     do {                                                                      \
>> -        strncpy(_suffix##_name, prefix, sizeof (_suffix##_name) - 1);         \
>> -        _suffix##_name[sizeof (_suffix##_name) - 1] = '\0';                   \
>> -        strncat(_suffix##_name, "."#_suffix,                                  \
>> -                sizeof (_suffix##_name) - 1 - strlen(_suffix##_name));        \
>> +        snprintf(_suffix##_name, sizeof(_suffix##_name), "%s."#_suffix, prefix); \
>>     } while (0)
> 
> The do/while can be removed as the macro expands to only one statement now:
> 
> #define ESX_BUILD_VMX_NAME(_suffix)                                           \
>     snprintf(_suffix##_name, sizeof(_suffix##_name), "%s."#_suffix, prefix)   \

Oops, of course you are right.  I'll fix that up.

> 
>> @@ -839,11 +836,9 @@ esxVMX_ParseSCSIController(virConnectPtr conn, virConfPtr conf, int controller,
>>         goto failure;
>>     }
>>
>> -    strncpy(present_name, "scsiX.present", sizeof (virtualDev_name));
>> -    strncpy(virtualDev_name, "scsiX.virtualDev", sizeof (virtualDev_name));
>> -
>> -    present_name[4] = '0' + controller;
>> -    virtualDev_name[4] = '0' + controller;
>> +    snprintf(present_name, sizeof(present_name), "scsi%d.present", controller);
>> +    snprintf(virtualDev_name, sizeof(virtualDev_name), "scsi%d.virtualDev",
>> +             controller);
>>
>>     if (esxUtil_GetConfigBoolean(conn, conf, present_name, present, 0, 1) < 0) {
>>         goto failure;
>> @@ -1333,8 +1328,7 @@ esxVMX_ParseEthernet(virConnectPtr conn, virConfPtr conf, int controller,
>>         goto failure;
>>     }
>>
>> -    strncpy(prefix, "ethernetX", sizeof (prefix));
>> -    prefix[8] = '0' + controller;
>> +    snprintf(prefix, sizeof(prefix), "ethernet%d", controller);
>>
>>     ESX_BUILD_VMX_NAME(present);
>>     ESX_BUILD_VMX_NAME(startConnected);
>> @@ -1514,8 +1508,7 @@ esxVMX_ParseSerial(virConnectPtr conn, virConfPtr conf, int port,
>>         goto failure;
>>     }
>>
>> -    strncpy(prefix, "serialX", sizeof (prefix));
>> -    prefix[6] = '0' + port;
>> +    snprintf(prefix, sizeof(prefix), "serial%d", port);
>>
>>     ESX_BUILD_VMX_NAME(present);
>>     ESX_BUILD_VMX_NAME(startConnected);
>> @@ -1627,8 +1620,7 @@ esxVMX_ParseParallel(virConnectPtr conn, virConfPtr conf, int port,
>>         goto failure;
>>     }
>>
>> -    strncpy(prefix, "parallelX", sizeof (prefix));
>> -    prefix[8] = '0' + port;
>> +    snprintf(prefix, sizeof(prefix), "parallel%d", port);
>>
>>     ESX_BUILD_VMX_NAME(present);
>>     ESX_BUILD_VMX_NAME(startConnected);
>> --
>> 1.6.0.6
>>
> 
> Just tested it, no objections beside the cosmetic one (do/while), ACK.

Great, thanks a lot!  I've now committed the code with the change you suggested,
and added a "Tested-by: Mattias Bolte <matthias.bolte at googlemail.com>" tag.

-- 
Chris Lalancette