Re: [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