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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list