15 out of 72 invocations of virStrcpy(Static) ignore the return value as it's either impossible to fail or in certain cases a truncated copy is still good enough. Unfortunately virStrcpy doesn't copy anything in such case as the checks are done first. Fix this by using g_strlcpy for the implementation and removing G_GNUC_WARN_UNUSED_RESULT from the function so that callers can decide when it's okay. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virstring.c | 12 +++++++----- src/util/virstring.h | 3 +-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/util/virstring.c b/src/util/virstring.c index c3e64007fe..a35cd8ba76 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -503,16 +503,18 @@ virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) * @src: source buffer * @destbytes: number of bytes the destination can accommodate * - * Copies @src to @dest. + * Copies @src to @dest. @dest is guaranteed to be 'nul' terminated if + * destbytes is 1 or more. * - * See virStrncpy() for more information. - * - * Returns: 0 on success, <0 on failure. + * Returns: 0 on success, -1 if @src doesn't fit into @dest and was truncated. */ int virStrcpy(char *dest, const char *src, size_t destbytes) { - return virStrncpy(dest, src, -1, destbytes); + if (g_strlcpy(dest, src, destbytes) >= destbytes) + return -1; + + return 0; } /** diff --git a/src/util/virstring.h b/src/util/virstring.h index 45aead1838..da1fe86ffc 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -99,8 +99,7 @@ bool virStringIsEmpty(const char *str); int virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) G_GNUC_WARN_UNUSED_RESULT; -int virStrcpy(char *dest, const char *src, size_t destbytes) - G_GNUC_WARN_UNUSED_RESULT; +int virStrcpy(char *dest, const char *src, size_t destbytes); #define virStrcpyStatic(dest, src) virStrcpy((dest), (src), sizeof(dest)) int virStringSortCompare(const void *a, const void *b); -- 2.29.2

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virprocess.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 69d64e9466..7a0960e337 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1168,14 +1168,14 @@ virProcessRunInForkHelper(int errfd, bin->data.code = err->code; bin->data.domain = err->domain; - ignore_value(virStrcpy(bin->data.message, err->message, sizeof(bin->data.message))); + virStrcpyStatic(bin->data.message, err->message); bin->data.level = err->level; if (err->str1) - ignore_value(virStrcpy(bin->data.str1, err->str1, sizeof(bin->data.str1))); + virStrcpyStatic(bin->data.str1, err->str1); if (err->str2) - ignore_value(virStrcpy(bin->data.str2, err->str2, sizeof(bin->data.str2))); + virStrcpyStatic(bin->data.str2, err->str2); if (err->str3) - ignore_value(virStrcpy(bin->data.str3, err->str3, sizeof(bin->data.str3))); + virStrcpyStatic(bin->data.str3, err->str3); bin->data.int1 = err->int1; bin->data.int2 = err->int2; -- 2.29.2

We want a (possibly truncated) copy of the full source string so virStrcpy is a better fit. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/locking/lock_driver_sanlock.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index aaffe30e6f..1d8f109375 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -924,9 +924,7 @@ 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, - MIN(strlen(priv->vm_name), SANLK_NAME_LEN - 1), - SANLK_NAME_LEN)); + virStrcpy(opt->owner_name, priv->vm_name, SANLK_NAME_LEN); if (state && STRNEQ(state, "")) { if ((rv = sanlock_state_to_args((char *)state, -- 2.29.2

virStrncpy was called with -1 for lenght of the copied source which is equivalent to virStrcpy. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virdevmapper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index d97334bf67..fcb11e954f 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -240,7 +240,7 @@ virDevMapperGetTargetsImpl(int controlFD, if (!(sanitizedPath = virDMSanitizepath(path))) return 0; - if (virStrncpy(dm.name, sanitizedPath, -1, DM_TABLE_DEPS) < 0) { + if (virStrcpy(dm.name, sanitizedPath, DM_TABLE_DEPS) < 0) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("Resolved device mapper name too long")); return -1; -- 2.29.2

Passing 'strlen(src)' for length makes it equivalent to virStrcpy. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virfile.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 841a0f4e00..29445fc0ff 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -810,8 +810,7 @@ int virFileLoopDeviceAssociate(const char *file, lo.lo_flags = LO_FLAGS_AUTOCLEAR; /* Set backing file name for LOOP_GET_STATUS64 queries */ - if (virStrncpy((char *) lo.lo_file_name, file, - strlen(file), LO_NAME_SIZE) < 0) { + if (virStrcpy((char *) lo.lo_file_name, file, LO_NAME_SIZE) < 0) { virReportSystemError(errno, _("Unable to set backing file %s"), file); goto cleanup; -- 2.29.2

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/libxl/xen_xm.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/libxl/xen_xm.c b/src/libxl/xen_xm.c index 3e81c9ef0d..cc24317a76 100644 --- a/src/libxl/xen_xm.c +++ b/src/libxl/xen_xm.c @@ -146,14 +146,7 @@ xenParseXMDisk(char *entry, int hvm) if (!(offset = strchr(head, ','))) goto error; - disk->dst = g_new0(char, (offset - head) + 1); - - if (virStrncpy(disk->dst, head, offset - head, - (offset - head) + 1) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Dest file %s too big for destination"), head); - goto error; - } + disk->dst = g_strndup(head, offset - head); head = offset + 1; /* Extract source driver type */ -- 2.29.2

Use glib functions to do the relative name lookup instead of manual assembly. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/test/test_driver.c | 35 ++++++----------------------------- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 71ab04aa1a..30a1959589 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -739,32 +739,14 @@ testDomainStartState(testDriverPtr privconn, static char *testBuildFilename(const char *relativeTo, const char *filename) { - char *offset; - int baseLen; - char *ret; + g_autofree char *basename = NULL; - if (!filename || filename[0] == '\0') - return NULL; - if (filename[0] == '/') { - ret = g_strdup(filename); - return ret; - } + if (g_path_is_absolute(filename)) + return g_strdup(filename); - offset = strrchr(relativeTo, '/'); - if ((baseLen = (offset-relativeTo+1))) { - char *absFile; - int totalLen = baseLen + strlen(filename) + 1; - absFile = g_new0(char, totalLen); - if (virStrncpy(absFile, relativeTo, baseLen, totalLen) < 0) { - VIR_FREE(absFile); - return NULL; - } - strcat(absFile, filename); - return absFile; - } else { - ret = g_strdup(filename); - return ret; - } + basename = g_path_get_dirname(relativeTo); + + return g_strdup_printf("%s/%s", basename, filename); } static xmlNodePtr @@ -777,11 +759,6 @@ testParseXMLDocFromFile(xmlNodePtr node, const char *file, const char *type) if ((relFile = virXMLPropString(node, "file"))) { absFile = testBuildFilename(file, relFile); - if (!absFile) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("resolving %s filename"), type); - return NULL; - } if (!(doc = virXMLParse(absFile, NULL, type))) goto error; -- 2.29.2

Use g_strsplit to split the string and avoid use of stack'd strings. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/libxl/xen_common.c | 136 ++++++++++++++--------------------------- 1 file changed, 46 insertions(+), 90 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 781483f496..09c74dcb53 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -1141,104 +1141,61 @@ xenParseVif(char *entry, const char *vif_typename) { virDomainNetDefPtr net = NULL; virDomainNetDefPtr ret = NULL; - char *script = NULL; - char model[10]; - char type[10]; - char ip[128]; - char mac[18]; - char bridge[50]; - char vifname[50]; - char rate[50]; - char *key; - - bridge[0] = '\0'; - mac[0] = '\0'; - ip[0] = '\0'; - model[0] = '\0'; - type[0] = '\0'; - vifname[0] = '\0'; - rate[0] = '\0'; - - key = entry; - while (key) { - char *data; - char *nextkey = strchr(key, ','); - - if (!(data = strchr(key, '='))) + g_auto(GStrv) keyvals = NULL; + GStrv keyval; + g_autofree char *script = NULL; + g_autofree char *model = NULL; + g_autofree char *type = NULL; + g_autofree char *ip = NULL; + g_autofree char *mac = NULL; + g_autofree char *bridge = NULL; + g_autofree char *vifname = NULL; + g_autofree char *rate = NULL; + + keyvals = g_strsplit(entry, ",", 0); + + for (keyval = keyvals; keyval && *keyval; keyval++) { + const char *key = *keyval; + char *val = strchr(key, '='); + + virSkipSpaces(&key); + + if (!val) return NULL; - data++; + + val++; if (STRPREFIX(key, "mac=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(mac, data, len, sizeof(mac)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("MAC address %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&mac, g_free); + mac = g_strdup(val); } else if (STRPREFIX(key, "bridge=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(bridge, data, len, sizeof(bridge)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Bridge %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&bridge, g_free); + bridge = g_strdup(val); } else if (STRPREFIX(key, "script=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - VIR_FREE(script); - script = g_strndup(data, len); + g_clear_pointer(&script, g_free); + script = g_strdup(val); } else if (STRPREFIX(key, "model=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(model, data, len, sizeof(model)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Model %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&model, g_free); + model = g_strdup(val); } else if (STRPREFIX(key, "type=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(type, data, len, sizeof(type)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Type %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&type, g_free); + type = g_strdup(val); } else if (STRPREFIX(key, "vifname=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(vifname, data, len, sizeof(vifname)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Vifname %s too big for destination"), - data); - return NULL; - } + g_clear_pointer(&vifname, g_free); + vifname = g_strdup(val); } else if (STRPREFIX(key, "ip=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(ip, data, len, sizeof(ip)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("IP %s too big for destination"), data); - return NULL; - } + g_clear_pointer(&ip, g_free); + ip = g_strdup(val); } else if (STRPREFIX(key, "rate=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(rate, data, len, sizeof(rate)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("rate %s too big for destination"), data); - return NULL; - } + g_clear_pointer(&rate, g_free); + rate = g_strdup(val); } - - while (nextkey && (nextkey[0] == ',' || - nextkey[0] == ' ' || - nextkey[0] == '\t')) - nextkey++; - key = nextkey; } if (!(net = virDomainNetDefNew(NULL))) goto cleanup; - if (mac[0]) { + if (mac) { if (virMacAddrParse(mac, &net->mac) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("malformed mac address '%s'"), mac); @@ -1246,18 +1203,18 @@ xenParseVif(char *entry, const char *vif_typename) } } - if (bridge[0] || STREQ_NULLABLE(script, "vif-bridge") || + if (bridge || STREQ_NULLABLE(script, "vif-bridge") || STREQ_NULLABLE(script, "vif-vnic")) { net->type = VIR_DOMAIN_NET_TYPE_BRIDGE; } else { net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; } - if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE && bridge[0]) { + if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE && bridge) { if (xenParseVifBridge(net, bridge) < 0) goto cleanup; } - if (ip[0]) { + if (ip) { char **ip_list = g_strsplit(ip, " ", 0); size_t i; @@ -1276,18 +1233,18 @@ xenParseVif(char *entry, const char *vif_typename) if (script && script[0]) net->script = g_strdup(script); - if (model[0]) { + if (model) { if (virDomainNetSetModelString(net, model) < 0) goto cleanup; } else { - if (type[0] && STREQ(type, vif_typename)) + if (type && STREQ(type, vif_typename)) net->model = VIR_DOMAIN_NET_MODEL_NETFRONT; } - if (vifname[0]) + if (vifname && vifname[0]) net->ifname = g_strdup(vifname); - if (rate[0]) { + if (rate) { virNetDevBandwidthPtr bandwidth; unsigned long long kbytes_per_sec; @@ -1304,7 +1261,6 @@ xenParseVif(char *entry, const char *vif_typename) cleanup: virDomainNetDefFree(net); - VIR_FREE(script); return ret; } -- 2.29.2

Copy the input string so that we don't have to use a static buffer and virStrncpy. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/libxl/xen_common.c | 46 +++++++++++++++++------------------------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 09c74dcb53..1c71f69209 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -1332,38 +1332,30 @@ xenParseSxprSound(virDomainDefPtr def, def->sounds[def->nsounds++] = sound; } } else { - char model[10]; - const char *offset = str, *offset2; - - do { - int len; - virDomainSoundDefPtr sound; - offset2 = strchr(offset, ','); - if (offset2) - len = (offset2 - offset); - else - len = strlen(offset); - if (virStrncpy(model, offset, len, sizeof(model)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Sound model %s too big for destination"), - offset); - return -1; - } + g_autofree char *sounds = g_strdup(str); + char *sound = sounds; + int model; - sound = g_new0(virDomainSoundDef, 1); + while (*sound != '\0') { + char *next = strchr(sound, ','); + virDomainSoundDefPtr snddef; - if ((sound->model = virDomainSoundModelTypeFromString(model)) < 0) { - VIR_FREE(sound); - return -1; - } + if (next) + *next = '\0'; - if (VIR_APPEND_ELEMENT(def->sounds, def->nsounds, sound) < 0) { - virDomainSoundDefFree(sound); + if ((model = virDomainSoundModelTypeFromString(sound)) < 0) return -1; - } - offset = offset2 ? offset2 + 1 : NULL; - } while (offset); + snddef = g_new0(virDomainSoundDef, 1); + snddef->model = model; + + ignore_value(VIR_APPEND_ELEMENT(def->sounds, def->nsounds, snddef)); + + if (!next) + break; + + sound = next + 1; + } } return 0; -- 2.29.2

Rewrite so that the parser doesn't use virStrncpy by employing g_strsplit. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/openvz/openvz_conf.c | 77 ++++++++++++++-------------------------- 1 file changed, 26 insertions(+), 51 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 041a031a3a..836885af18 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -230,82 +230,57 @@ openvzReadNetworkConf(virDomainDefPtr def, veid); goto error; } else if (ret > 0) { - token = strtok_r(temp, ";", &saveptr); - while (token != NULL) { - char *p = token; - char cpy_temp[32]; - int len; + g_auto(GStrv) devices = g_strsplit(temp, ";", 0); + GStrv device; - /* add new device to list */ - net = g_new0(virDomainNetDef, 1); + for (device = devices; device && *device; device++) { + g_auto(GStrv) keyvals = g_strsplit(*device, ",", 0); + GStrv keyval; + net = g_new0(virDomainNetDef, 1); net->type = VIR_DOMAIN_NET_TYPE_BRIDGE; - /* parse string */ - do { - char *next = strchr(p, ','); - if (!next) - next = strchr(p, '\0'); - if (STRPREFIX(p, "ifname=")) { + for (keyval = keyvals; keyval && *keyval; keyval++) { + char *val = strchr(*keyval, '='); + + if (!val) + continue; + + val++; + + if (STRPREFIX(*keyval, "ifname=")) { /* skip in libvirt */ - } else if (STRPREFIX(p, "host_ifname=")) { - p += 12; - len = next - p; - if (len > 16) { + } else if (STRPREFIX(*keyval, "host_ifname=")) { + if (strlen(val) > 16) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Too long network device name")); goto error; } - net->ifname = g_new0(char, len+1); - - if (virStrncpy(net->ifname, p, len, len+1) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network ifname %s too long for destination"), p); - goto error; - } - } else if (STRPREFIX(p, "bridge=")) { - p += 7; - len = next - p; - if (len > 16) { + net->ifname = g_strdup(val); + } else if (STRPREFIX(*keyval, "bridge=")) { + if (strlen(val) > 16) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Too long bridge device name")); goto error; } - net->data.bridge.brname = g_new0(char, len+1); - - if (virStrncpy(net->data.bridge.brname, p, len, len+1) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Bridge name %s too long for destination"), p); - goto error; - } - } else if (STRPREFIX(p, "mac=")) { - p += 4; - len = next - p; - if (len != 17) { /* should be 17 */ + net->data.bridge.brname = g_strdup(val); + } else if (STRPREFIX(*keyval, "mac=")) { + if (strlen(val) != 17) { /* should be 17 */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Wrong length MAC address")); goto error; } - if (virStrncpy(cpy_temp, p, len, sizeof(cpy_temp)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("MAC address %s too long for destination"), p); - goto error; - } - if (virMacAddrParse(cpy_temp, &net->mac) < 0) { + if (virMacAddrParse(val, &net->mac) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Wrong MAC address")); goto error; } } - p = ++next; - } while (p < token + strlen(token)); - - if (VIR_APPEND_ELEMENT_COPY(def->nets, def->nnets, net) < 0) - goto error; + } - token = strtok_r(NULL, ";", &saveptr); + ignore_value(VIR_APPEND_ELEMENT_COPY(def->nets, def->nnets, net)); } } -- 2.29.2

Make the temporary string a autofree-ing pointer and copy the contents. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/libxl/xen_xl.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 4113be8cd1..9b2a2fe292 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -1070,7 +1070,7 @@ xenParseXLChannel(virConfPtr conf, virDomainDefPtr def) if (list && list->type == VIR_CONF_LIST) { list = list->list; while (list) { - char type[10]; + g_autofree char *type = NULL; char *key; if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) @@ -1087,11 +1087,8 @@ xenParseXLChannel(virConfPtr conf, virDomainDefPtr def) if (STRPREFIX(key, "connection=")) { int len = nextkey ? (nextkey - data) : strlen(data); - if (virStrncpy(type, data, len, sizeof(type)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("connection %s too big"), data); - goto skipchannel; - } + g_clear_pointer(&type, g_free); + type = g_strndup(data, len); } else if (STRPREFIX(key, "name=")) { int len = nextkey ? (nextkey - data) : strlen(data); VIR_FREE(name); -- 2.29.2