Re: [libvirt] [PATCH] Introduce virStrncpy.
2009/8/28 Chris Lalancette <clalance(a)redhat.com>:
Add the virStrncpy function, which takes a dst string, source
string,
the number of bytes to copy and the number of bytes available in the
dest string. If the source string is too large to fit into the
destination string, including the \0 byte, then no data is copied and
the function returns NULL. Otherwise, this function copies n bytes
from source into dst, including the \0, and returns a pointer to the
dst string. This function is intended to replace all unsafe uses
of strncpy in the code base, since strncpy does *not* guarantee that
the buffer terminates with a \0.
[...]
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;
+ }
I looked at the rest of the changes from strncpy to virStrcpyStatic,
but they seem to be okay and not breaking existing behavior.
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.
Matthias
Attachments:
- esx_remove_strncpy_from_esxVMX_IndexToDiskName.patch (application/mbox — 1.5 KB)