6:09 a.m.
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
6:09 a.m.
New subject: [libvirt] [PATCH 1/3] src: Use virStrcpyStatic() to avoid truncation
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(a)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
6:09 a.m.
New subject: [libvirt] [PATCH 2/3] src: Use virStrcpyStatic() wherever possible
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(a)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
6:09 a.m.
New subject: [libvirt] [PATCH 3/3] m4: Work around MinGW detection of strncpy() usage
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(a)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
6:23 a.m.
New subject: [libvirt] [PATCH 3/3] m4: Work around MinGW detection of strncpy() usage
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(a)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 :|
7:20 a.m.
New subject: [libvirt] [PATCH 3/3] m4: Work around MinGW detection of strncpy() usage
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
2323
days inactive
2323
days old
5 comments
2 participants
participants (2)
-
Andrea Bolognani -
Daniel P. Berrangé