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