src: Don't rely on strncpy()-like behavior

The strncpy() function has this quirk where it will copy
*up* to the requested number of bytes, that is, it will
stop early if it encounters a NULL byte in the source
string.

This makes it legal to pass the size of the destination
buffer (minus one byte needed for the string terminator)
as the number of bytes to copy and still get something
somewhat reasonable out of the operation; unfortunately,
it also makes the function difficult to reason about
and way too easy to misuse.

We want to move away from the way strncpy() behaves and
towards better defined semantics, where virStrncpy()
will always copy *exactly* the number of bytes it's
been asked to copy; before we can do that, though, we
have to change a few of the callers.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>

diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
index 345cf0a7725776191ebebf8475c9d75f320ed2f5..3f3a58754191ab3f940433a79ef37bd77cff49a3 100644 (file)
--- a/src/locking/lock_driver_sanlock.c
+++ b/src/locking/lock_driver_sanlock.c
@@ -1004,7 +1004,8 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,
     /* sanlock doesn't use owner_name for anything, so it's safe to take just
      * the first SANLK_NAME_LEN - 1 characters from vm_name */
     ignore_value(virStrncpy(opt->owner_name, priv->vm_name,
-                            SANLK_NAME_LEN - 1, SANLK_NAME_LEN));
+                            MIN(strlen(priv->vm_name), SANLK_NAME_LEN - 1),
+                            SANLK_NAME_LEN));
 
     if (state && STRNEQ(state, "")) {
         if ((rv = sanlock_state_to_args((char *)state,

diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c
index 42b305d3169c0c5edbbd4d1c72181e0c213e646b..f4375c587429f92d6058369d3d9d23285054f515 100644 (file)
--- a/src/xenapi/xenapi_driver.c
+++ b/src/xenapi/xenapi_driver.c
@@ -430,7 +430,9 @@ xenapiNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
     if (xen_host_cpu_get_all(session, &host_cpu_set)) {
         host_cpu = host_cpu_set->contents[0];
         xen_host_cpu_get_modelname(session, &modelname, host_cpu);
-        if (!virStrncpy(info->model, modelname, LIBVIRT_MODELNAME_LEN - 1, LIBVIRT_MODELNAME_LEN)) {
+        if (!virStrncpy(info->model, modelname,
+                        MIN(strlen(modelname), LIBVIRT_MODELNAME_LEN - 1),
+                        LIBVIRT_MODELNAME_LEN)) {
             virReportOOMError();
             xen_host_cpu_set_free(host_cpu_set);
             VIR_FREE(modelname);

diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 4a94127da10c73bfd56f35aeefe324c94de2a354..815ccd030e3c022d6fbfc8c701b001cbd44e9009 100644 (file)
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -879,7 +879,7 @@ xenParseVif(char *entry, const char *vif_typename)
         data++;
 
         if (STRPREFIX(key, "mac=")) {
-            int len = nextkey ? (nextkey - data) : sizeof(mac) - 1;
+            int len = nextkey ? (nextkey - data) : strlen(data);
             if (virStrncpy(mac, data, len, sizeof(mac)) == NULL) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("MAC address %s too big for destination"),
@@ -887,7 +887,7 @@ xenParseVif(char *entry, const char *vif_typename)
                 return NULL;
             }
         } else if (STRPREFIX(key, "bridge=")) {
-            int len = nextkey ? (nextkey - data) : sizeof(bridge) - 1;
+            int len = nextkey ? (nextkey - data) : strlen(data);
             if (virStrncpy(bridge, data, len, sizeof(bridge)) == NULL) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("Bridge %s too big for destination"),
@@ -900,7 +900,7 @@ xenParseVif(char *entry, const char *vif_typename)
             if (VIR_STRNDUP(script, data, len) < 0)
                 return NULL;
         } else if (STRPREFIX(key, "model=")) {
-            int len = nextkey ? (nextkey - data) : sizeof(model) - 1;
+            int len = nextkey ? (nextkey - data) : strlen(data);
             if (virStrncpy(model, data, len, sizeof(model)) == NULL) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("Model %s too big for destination"),
@@ -908,7 +908,7 @@ xenParseVif(char *entry, const char *vif_typename)
                 return NULL;
             }
         } else if (STRPREFIX(key, "type=")) {
-            int len = nextkey ? (nextkey - data) : sizeof(type) - 1;
+            int len = nextkey ? (nextkey - data) : strlen(data);
             if (virStrncpy(type, data, len, sizeof(type)) == NULL) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("Type %s too big for destination"),
@@ -916,7 +916,7 @@ xenParseVif(char *entry, const char *vif_typename)
                 return NULL;
             }
         } else if (STRPREFIX(key, "vifname=")) {
-            int len = nextkey ? (nextkey - data) : sizeof(vifname) - 1;
+            int len = nextkey ? (nextkey - data) : strlen(data);
             if (virStrncpy(vifname, data, len, sizeof(vifname)) == NULL) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("Vifname %s too big for destination"),
@@ -924,14 +924,14 @@ xenParseVif(char *entry, const char *vif_typename)
                 return NULL;
             }
         } else if (STRPREFIX(key, "ip=")) {
-            int len = nextkey ? (nextkey - data) : sizeof(ip) - 1;
+            int len = nextkey ? (nextkey - data) : strlen(data);
             if (virStrncpy(ip, data, len, sizeof(ip)) == NULL) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("IP %s too big for destination"), data);
                 return NULL;
             }
         } else if (STRPREFIX(key, "rate=")) {
-            int len = nextkey ? (nextkey - data) : sizeof(rate) - 1;
+            int len = nextkey ? (nextkey - data) : strlen(data);
             if (virStrncpy(rate, data, len, sizeof(rate)) == NULL) {
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("rate %s too big for destination"), data);

diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index 807fe621d6ebf7d9aad25b0387630e89bbc8f6b5..bc3191ad5ea645e0a46d2cfbb99533be41e51ca4 100644 (file)
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -899,7 +899,7 @@ xenParseXLUSBController(virConfPtr conf, virDomainDefPtr def)
                 data++;
 
                 if (STRPREFIX(key, "type=")) {
-                    int len = nextkey ? (nextkey - data) : sizeof(type) - 1;
+                    int len = nextkey ? (nextkey - data) : strlen(data);
                     if (virStrncpy(type, data, len, sizeof(type)) == NULL) {
                         virReportError(VIR_ERR_INTERNAL_ERROR,
                                        _("type %s invalid"),
@@ -907,7 +907,7 @@ xenParseXLUSBController(virConfPtr conf, virDomainDefPtr def)
                         goto skipusbctrl;
                     }
                 } else if (STRPREFIX(key, "version=")) {
-                    int len = nextkey ? (nextkey - data) : sizeof(version) - 1;
+                    int len = nextkey ? (nextkey - data) : strlen(data);
                     if (virStrncpy(version, data, len, sizeof(version)) == NULL) {
                         virReportError(VIR_ERR_INTERNAL_ERROR,
                                        _("version %s invalid"),
@@ -917,7 +917,7 @@ xenParseXLUSBController(virConfPtr conf, virDomainDefPtr def)
                     if (virStrToLong_i(version, NULL, 16, &usbctrl_version) < 0)
                         goto skipusbctrl;
                 } else if (STRPREFIX(key, "ports=")) {
-                    int len = nextkey ? (nextkey - data) : sizeof(ports) - 1;
+                    int len = nextkey ? (nextkey - data) : strlen(data);
                     if (virStrncpy(ports, data, len, sizeof(ports)) == NULL) {
                         virReportError(VIR_ERR_INTERNAL_ERROR,
                                        _("version %s invalid"),
@@ -1001,7 +1001,7 @@ xenParseXLUSB(virConfPtr conf, virDomainDefPtr def)
                 data++;
 
                 if (STRPREFIX(key, "hostbus=")) {
-                    int len = nextkey ? (nextkey - data) : sizeof(bus) - 1;
+                    int len = nextkey ? (nextkey - data) : strlen(data);
                     if (virStrncpy(bus, data, len, sizeof(bus)) == NULL) {
                         virReportError(VIR_ERR_INTERNAL_ERROR,
                                        _("bus %s too big for destination"),
@@ -1009,7 +1009,7 @@ xenParseXLUSB(virConfPtr conf, virDomainDefPtr def)
                         goto skipusb;
                     }
                 } else if (STRPREFIX(key, "hostaddr=")) {
-                    int len = nextkey ? (nextkey - data) : sizeof(device) - 1;
+                    int len = nextkey ? (nextkey - data) : strlen(data);
                     if (virStrncpy(device, data, len, sizeof(device)) == NULL) {
                         virReportError(VIR_ERR_INTERNAL_ERROR,
                                        _("device %s too big for destination"),
@@ -1077,7 +1077,7 @@ xenParseXLChannel(virConfPtr conf, virDomainDefPtr def)
                 data++;
 
                 if (STRPREFIX(key, "connection=")) {
-                    int len = nextkey ? (nextkey - data) : sizeof(type) - 1;
+                    int len = nextkey ? (nextkey - data) : strlen(data);
                     if (virStrncpy(type, data, len, sizeof(type)) == NULL) {
                         virReportError(VIR_ERR_INTERNAL_ERROR,
                                        _("connection %s too big"), data);