[libvirt] [PATCH v2 5/8] 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(a)redhat.com&gt; --- src/locking/lock_driver_sanlock.c | 3 ++- src/xenapi/xenapi_driver.c | 4 +++- src/xenconfig/xen_common.c | 14 +++++++------- src/xenconfig/xen_xl.c | 12 ++++++------ 4 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 345cf0a772..3f3a587541 100644 --- 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 42b305d316..f4375c5874 100644 --- 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 4a94127da1..815ccd030e 100644 --- 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 807fe621d6..bc3191ad5e 100644 --- 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); -- 2.17.1