[libvirt] [PATCH] Introduce virStrncpy.

[Chris Lalancette]

Matthias Bolte wrote:
>> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
>> index 0225e9a..24c4422 100644
>> --- a/src/esx/esx_driver.c
>> +++ b/src/esx/esx_driver.c
>> @@ -694,9 +694,12 @@ esxNodeGetInfo(virConnectPtr conn, virNodeInfoPtr nodeinfo)
>>                 ++ptr;
>>             }
>>
>> -            strncpy (nodeinfo->model, dynamicProperty->val->string,
>> -                     sizeof (nodeinfo->model) - 1);
>> -            nodeinfo->model[sizeof (nodeinfo->model) - 1] = '\0';
>> +            if (virStrcpyStatic(nodeinfo->model, dynamicProperty->val->string) == NULL) {
>> +                ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
>> +                          "Model %s too long for destination",
>> +                          dynamicProperty->val->string);
>> +                goto failure;
>> +            }
>>         } else {
>>             VIR_WARN("Unexpected '%s' property", dynamicProperty->name);
>>         }
> 
> NACK to this part of the patch.
> 
> For our testing hardware I get "Intel(R) Xeon(R) CPU E5345 @ 2.33GHz"
> as CPU model. Because ESX may report something like this as the CPU
> model string, that is longer than the 31 characters available in the
> virNodeInfo struct for it, I added some code that strips irrelevant
> characters like sequences of spaces of "(R)" from it. This way I can
> fit more relevant information into the 31 characters.
> 
> The original code would just take the first 31 characters of the
> striped string, put that into virNodeInfo, and null-terminate it
> properly. If the CPU model string was longer than 31 characters the
> rest would just have been chopped of:
> 
> strncpy (nodeinfo->model, dynamicProperty->val->string, sizeof
> (nodeinfo->model) - 1);
> nodeinfo->model[sizeof (nodeinfo->model) - 1] = '\0';
> 
> Your change to make it use virStrcpyStatic now breaks this behavior.
> Now if the CPU model string is longer than 31 characters, the call to
> esxNodeGetInfo will fail, rendering it unusable on systems with too
> long CPU model string.
> 
> I suggest the following change, that preserves the original behavior:
> 
> -            strncpy (nodeinfo->model, dynamicProperty->val->string,
> -                     sizeof (nodeinfo->model) - 1);
> -            nodeinfo->model[sizeof (nodeinfo->model) - 1] = '\0';
> +            if (virStrncpy(nodeinfo->model, sizeof (nodeinfo->model) - 1,
> +                           dynamicProperty->val->string,
> +                           sizeof (nodeinfo->model)) == NULL) {
> +                ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
> +                          "CPU Model '%s' too long for destination",
> +                          dynamicProperty->val->string);
> +                goto failure;
> +            }

Ah, thanks for this review and response, that's the sort of thing I was looking
for.  I tried to preserve this behavior where I could, but I obviously missed
(at least) this one.  I'll fix it up for the next submission.

>> diff --git a/src/esx/esx_vmx.c b/src/esx/esx_vmx.c
>> index 54c2594..2a58fd8 100644
>> --- a/src/esx/esx_vmx.c
>> +++ b/src/esx/esx_vmx.c
>> @@ -883,8 +883,11 @@ esxVMX_IndexToDiskName(virConnectPtr conn, int idx, const char *prefix)
>>         return NULL;
>>     }
>>
>> -    strncpy(buffer, prefix, sizeof (buffer) - 1);
>> -    buffer[sizeof (buffer) - 1] = '\0';
>> +    if (virStrcpyStatic(buffer, prefix) == NULL) {
>> +        ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR,
>> +                  "Prefix %s too big for destination", prefix);
>> +        return NULL;
>> +    }
>>
>>     if (idx < 26) {
>>         buffer[length] = 'a' + idx;
> 
> Instead of double checking the buffer size here, I suggest a rewrite
> of this function that doesn't use strncpy at all. Because the buffer
> is strdup'ed afterwards anyway, the function could also use
> virAsprintf to build the result. See the attached patch.

Cool.  I'll fold this into a resubmitted patch as well.

Thanks again!

-- 
Chris Lalancette