[libvirt] [PATCH 0/3] virStr*cpy*() related fixes and cleanups
1/3 contains bug fixes, 2/3 a bunch of cleanups and 3/3 makes libvirt build again on MinGW. Andrea Bolognani (3): src: Use virStrcpyStatic() to avoid truncation src: Use virStrcpyStatic() wherever possible m4: Work around MinGW detection of strncpy() usage cfg.mk | 2 +- m4/virt-compile-warnings.m4 | 5 +++++ src/conf/nwfilter_conf.c | 3 +-- src/esx/esx_driver.c | 4 +--- src/hyperv/hyperv_driver.c | 3 +-- src/util/virfdstream.c | 2 +- src/util/virlog.c | 5 ++--- src/util/virnetdev.c | 3 +-- src/xenconfig/xen_xl.c | 17 ++++------------- 9 files changed, 17 insertions(+), 27 deletions(-) -- 2.17.1
The way virStrncpy() is called here will never result in buffer overflow, but it won't prevent or detect truncation either, despite what the error message might suggest. Use virStrcpyStatic(), which does all of the above, instead. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/esx/esx_driver.c | 4 +--- src/hyperv/hyperv_driver.c | 3 +-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 947b7c1a31..edd21b9d28 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -1317,9 +1317,7 @@ esxNodeGetInfo(virConnectPtr conn, virNodeInfoPtr nodeinfo) ++ptr; } - if (!virStrncpy(nodeinfo->model, dynamicProperty->val->string, - sizeof(nodeinfo->model) - 1, - sizeof(nodeinfo->model))) { + if (!virStrcpyStatic(nodeinfo->model, dynamicProperty->val->string)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("CPU Model %s too long for destination"), dynamicProperty->val->string); diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index a85943668c..6f74adf372 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -307,8 +307,7 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info) } /* Fill struct */ - if (virStrncpy(info->model, processorList->data.common->Name, - sizeof(info->model) - 1, sizeof(info->model)) == NULL) { + if (virStrcpyStatic(info->model, processorList->data.common->Name) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("CPU model %s too long for destination"), processorList->data.common->Name); -- 2.17.1
This convenience macro was created for the simple cases where the length of the source string and the size of the destination buffer can be figued out with strlen() and sizeof() respectively, so we should use it wherever possible instead of open-coding parts of it. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/nwfilter_conf.c | 3 +-- src/util/virfdstream.c | 2 +- src/util/virlog.c | 5 ++--- src/util/virnetdev.c | 3 +-- src/xenconfig/xen_xl.c | 17 ++++------------- 5 files changed, 9 insertions(+), 21 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 706e803a25..36a7315880 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -966,8 +966,7 @@ ipsetValidator(enum attrDatatype datatype ATTRIBUTE_UNUSED, { const char *errmsg = NULL; - if (virStrcpy(item->u.ipset.setname, val->c, - sizeof(item->u.ipset.setname)) == NULL) { + if (virStrcpyStatic(item->u.ipset.setname, val->c) == NULL) { errmsg = _("ipset name is too long"); goto arg_err_exit; } diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 8189559964..f4777cfd12 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -1183,7 +1183,7 @@ int virFDStreamConnectUNIX(virStreamPtr st, goto error; sa.sun_path[0] = '\0'; } else { - if (virStrcpy(sa.sun_path, path, sizeof(sa.sun_path)) == NULL) + if (virStrcpyStatic(sa.sun_path, path) == NULL) goto error; } diff --git a/src/util/virlog.c b/src/util/virlog.c index e008dd9c54..9d569057ae 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -284,8 +284,7 @@ virLogOnceInit(void) */ r = gethostname(virLogHostname, sizeof(virLogHostname)); if (r == -1) { - ignore_value(virStrcpy(virLogHostname, - "(unknown)", sizeof(virLogHostname))); + ignore_value(virStrcpyStatic(virLogHostname, "(unknown)")); } else { NUL_TERMINATE(virLogHostname); } @@ -1027,7 +1026,7 @@ virLogOutputToJournald(virLogSourcePtr source, memset(&sa, 0, sizeof(sa)); sa.sun_family = AF_UNIX; - if (!virStrcpy(sa.sun_path, "/run/systemd/journal/socket", sizeof(sa.sun_path))) + if (!virStrcpyStatic(sa.sun_path, "/run/systemd/journal/socket")) return; memset(&mh, 0, sizeof(mh)); diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index c20022fbc9..57ebd0ec03 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -914,8 +914,7 @@ int virNetDevGetIndex(const char *ifname, int *ifindex) memset(&ifreq, 0, sizeof(ifreq)); - if (virStrncpy(ifreq.ifr_name, ifname, strlen(ifname), - sizeof(ifreq.ifr_name)) == NULL) { + if (virStrcpyStatic(ifreq.ifr_name, ifname) == NULL) { virReportSystemError(ERANGE, _("invalid interface name %s"), ifname); diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index f0d9177cec..807fe621d6 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -475,15 +475,12 @@ xenParseXLVnuma(virConfPtr conf, data++; if (*data) { - size_t len; char vtoken[64]; if (STRPREFIX(str, "pnode")) { unsigned int cellid; - len = strlen(data); - if (!virStrncpy(vtoken, data, - len, sizeof(vtoken))) { + if (!virStrcpyStatic(vtoken, data)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("vnuma vnode %zu pnode '%s' too long for destination"), vnodeCnt, data); @@ -499,9 +496,7 @@ xenParseXLVnuma(virConfPtr conf, } pnode = cellid; } else if (STRPREFIX(str, "size")) { - len = strlen(data); - if (!virStrncpy(vtoken, data, - len, sizeof(vtoken))) { + if (!virStrcpyStatic(vtoken, data)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("vnuma vnode %zu size '%s' too long for destination"), vnodeCnt, data); @@ -514,9 +509,7 @@ xenParseXLVnuma(virConfPtr conf, virDomainNumaSetNodeMemorySize(numa, vnodeCnt, (kbsize * 1024)); } else if (STRPREFIX(str, "vcpus")) { - len = strlen(data); - if (!virStrncpy(vtoken, data, - len, sizeof(vtoken))) { + if (!virStrcpyStatic(vtoken, data)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("vnuma vnode %zu vcpus '%s' too long for destination"), vnodeCnt, data); @@ -533,9 +526,7 @@ xenParseXLVnuma(virConfPtr conf, size_t i, ndistances; unsigned int value; - len = strlen(data); - if (!virStrncpy(vtoken, data, - len, sizeof(vtoken))) { + if (!virStrcpyStatic(vtoken, data)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("vnuma vnode %zu vdistances '%s' too long for destination"), vnodeCnt, data); -- 2.17.1
With the recent update in Fedora Rawhide, MinGW has started freaking out about our use of strncpy(): In function 'virStrncpy', inlined from 'virStrcpy' at ../../src/util/virstring.c:811:12: ../../src/util/virstring.c:789:11: error: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Werror=stringop-truncation] ret = strncpy(dest, src, n); ^~~~~~~~~~~~~~~~~~~~~ ../../src/util/virstring.c: In function 'virStrcpy': ../../src/util/virstring.c:811:12: note: length computed here return virStrncpy(dest, src, strlen(src), destbytes); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ What the compiler is not detecting is that we perform proper bound checking right before calling the function, which makes our use of it perfectly safe. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- Kind of a big hammer, so if you have a better approach in mind please don't hesitate to step forward. cfg.mk | 2 +- m4/virt-compile-warnings.m4 | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index 609ae869c2..d059f803eb 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1240,7 +1240,7 @@ exclude_file_name_regexp--sc_prohibit_setuid = ^src/util/virutil\.c$$ exclude_file_name_regexp--sc_prohibit_sprintf = \ ^(cfg\.mk|docs/hacking\.html\.in|.*\.stp|.*\.pl)$$ -exclude_file_name_regexp--sc_prohibit_strncpy = ^src/util/virstring\.c$$ +exclude_file_name_regexp--sc_prohibit_strncpy = ^(src/util/virstring\.c|m4/virt-compile-warnings\.m4)$$ exclude_file_name_regexp--sc_prohibit_strtol = ^examples/.*$$ diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index fc185aef38..7d71cf2504 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -243,6 +243,11 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ wantwarn="$wantwarn -Wno-suggest-attribute=pure" wantwarn="$wantwarn -Wno-suggest-attribute=const" + # MinGW freaks out about our use of strncpy(), but we perform proper + # bound checking in our wrappers and prevent the underlying POSIX + # functions from being used directly through syntax-check + wantwarn="$wantwarn -Wno-stringop-truncation -Wno-stringop-overflow" + if test "$enable_werror" = "yes" then wantwarn="$wantwarn -Werror" -- 2.17.1
On Tue, Jul 17, 2018 at 01:09:57PM +0200, Andrea Bolognani wrote:
With the recent update in Fedora Rawhide, MinGW has started freaking out about our use of strncpy():
In function 'virStrncpy', inlined from 'virStrcpy' at ../../src/util/virstring.c:811:12: ../../src/util/virstring.c:789:11: error: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Werror=stringop-truncation] ret = strncpy(dest, src, n); ^~~~~~~~~~~~~~~~~~~~~ ../../src/util/virstring.c: In function 'virStrcpy': ../../src/util/virstring.c:811:12: note: length computed here return virStrncpy(dest, src, strlen(src), destbytes); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The GCC docs for this warning suggest that we should use memcpy() instead of strncpy() when we know that we might truncate. This looks simple enough given that we know the target buffer size and the input size. The caveat is whether any callers are providing a value of 'n' to virStrncpy() that exceeds the size of the 'src', and are thus relying on early termination when reaching '\0' ?
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- Kind of a big hammer, so if you have a better approach in mind please don't hesitate to step forward.
The smaller hammer is to just use pragma to turn off the warning around that single piece of code. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 2018-07-17 at 12:23 +0100, Daniel P. Berrangé wrote:
On Tue, Jul 17, 2018 at 01:09:57PM +0200, Andrea Bolognani wrote:
With the recent update in Fedora Rawhide, MinGW has started freaking out about our use of strncpy():
In function 'virStrncpy', inlined from 'virStrcpy' at ../../src/util/virstring.c:811:12: ../../src/util/virstring.c:789:11: error: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Werror=stringop-truncation] ret = strncpy(dest, src, n); ^~~~~~~~~~~~~~~~~~~~~ ../../src/util/virstring.c: In function 'virStrcpy': ../../src/util/virstring.c:811:12: note: length computed here return virStrncpy(dest, src, strlen(src), destbytes); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The GCC docs for this warning suggest that we should use memcpy() instead of strncpy() when we know that we might truncate. This looks simple enough given that we know the target buffer size and the input size.
The caveat is whether any callers are providing a value of 'n' to virStrncpy() that exceeds the size of the 'src', and are thus relying on early termination when reaching '\0' ?
There were a couple, and I got rid of them in patch 1/3 because their use of virStrncpy() could lead to silent truncation. Overall, I wouldn't be at all surprised if at some point someone introduced other similar callers because they expect our strncpy() wrapper, named after strncpy(), to behave like strncpy() ;) Too bad strncpy() semantics are so confusing, mostly due to the fact that there is a single length argument that basically refers both to the source and the destination; our wrappers fix this by adding a second argument, so that we can tweak them separately. Perhaps we should rename it to virStrcpyPrefix(), virStrcpyLength() or something similar to avoid confusion, and give it better semantics: * the destination buffer is always NULL-terminated (this is already the case for our wrappers, actually); * the length passed in can't exceed the length of the source string. The second one will require us to have some ad-hoc handling in esxVI_CURL_Debug(), but I think that's a reasonable compromise if it allows us to use better semantics everywhere else. We could improve the usage, and make it even clearer that strncpy() semantics should not be expected, by changing the return type from the pointless char * it is now to the more standard (at least as far as libvirt is concerned :) int.
Kind of a big hammer, so if you have a better approach in mind please don't hesitate to step forward.
The smaller hammer is to just use pragma to turn off the warning around that single piece of code.
If you like the proposal above, I'll start working on that instead. Feel free to review the first two patches of the series in the meantime ;) -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Daniel P. Berrangé