[libvirt] [PATCH] Introduce virStrncpy.

[Matthias Bolte]

2009/8/28 Chris Lalancette <clalance at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: esx_remove_strncpy_from_esxVMX_IndexToDiskName.patch
Type: application/mbox
Size: 1527 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20090829/6f1febc7/attachment-0001.mbox>