[libvirt PATCH 0/5] Miscelaneous libc-related fixes
Compiling libvirt on alpine showed a bunch of things that are potentially wrong, we just haven't noticed elsewhere. Martin Kletzander (5): syntax-check: Rework mock-noinline to get all files at once Include poll.h instead of sys/poll.h Include sys/wait.h instead of wait.h nwfilter: Avoid memory alignment issues tests: Allow expansion of mocked stat symbols build-aux/syntax-check.mk | 2 +- scripts/mock-noinline.py | 2 +- src/lxc/lxc_driver.c | 4 ++-- src/network/bridge_driver.c | 2 +- src/nwfilter/nwfilter_learnipaddr.c | 15 ++++++++------- src/openvz/openvz_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/security/security_apparmor.c | 2 +- src/vz/vz_driver.c | 2 +- tests/virmock.h | 4 +++- 10 files changed, 20 insertions(+), 17 deletions(-) -- 2.35.1
The script can break if the number of files does not fit one invocation and xargs has to split it. Instead pipe the list of files directly into the script and in the script read them from stdin instead of the arguments. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- build-aux/syntax-check.mk | 2 +- scripts/mock-noinline.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 9407581d0eb3..a8c9153b20ae 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1549,7 +1549,7 @@ sc_spacing-check: { echo '$(ME): incorrect formatting' 1>&2; exit 1; } sc_mock-noinline: - $(AM_V_GEN)$(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$' | $(RUNUTF8) xargs \ + $(AM_V_GEN)$(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$' | $(RUNUTF8) \ $(PYTHON) $(top_srcdir)/scripts/mock-noinline.py sc_header-ifdef: diff --git a/scripts/mock-noinline.py b/scripts/mock-noinline.py index 712550cb7a4c..13c296603d3a 100644 --- a/scripts/mock-noinline.py +++ b/scripts/mock-noinline.py @@ -63,7 +63,7 @@ def scan_overrides(filename): mocked[name] = "%s:%d" % (filename, lineno) -for filename in sys.argv[1:]: +for filename in sys.stdin.readlines(): if filename.endswith(".h"): scan_annotations(filename) elif filename.endswith("mock.c"): -- 2.35.1
That is the proper POSIX way. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/lxc/lxc_driver.c | 2 +- src/network/bridge_driver.c | 2 +- src/openvz/openvz_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/vz/vz_driver.c | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 020ec257aef1..8c041540ab47 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -30,7 +30,7 @@ #endif #include <sys/stat.h> -#include <sys/poll.h> +#include <poll.h> #include <unistd.h> #include <wait.h> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index d6ae05360b24..67c5a111b350 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -22,7 +22,7 @@ #include <config.h> #include <sys/types.h> -#include <sys/poll.h> +#include <poll.h> #include <stdarg.h> #include <unistd.h> #include <sys/utsname.h> diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index aa1db095400b..53214586a550 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -24,7 +24,7 @@ #include <config.h> #include <sys/types.h> -#include <sys/poll.h> +#include <poll.h> #include <stdarg.h> #include <unistd.h> #include <sys/stat.h> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8337eed510d8..812a900fa8b5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -22,7 +22,7 @@ #include <config.h> #include <sys/types.h> -#include <sys/poll.h> +#include <poll.h> #include <sys/time.h> #include <dirent.h> #include <stdarg.h> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index fc91b6dddf45..fdd776282e82 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -24,7 +24,7 @@ #include <config.h> #include <sys/types.h> -#include <sys/poll.h> +#include <poll.h> #include <stdarg.h> #include <unistd.h> #include <sys/stat.h> -- 2.35.1
On Mon, Mar 07, 2022 at 10:04:03AM +0100, Martin Kletzander wrote:
That is the proper POSIX way.
Indeed, but if someone wants to do more cleanup we should just stop using poll, and call g_poll instead. This is a transparent wrapper of poll on systems which have it, or a compatible impl on systems lacking it. With g_poll we don't need to care about poll.h at all. 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 :|
That is the proper POSIX way. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/lxc/lxc_driver.c | 2 +- src/security/security_apparmor.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 8c041540ab47..84d1f3935863 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -32,7 +32,7 @@ #include <sys/stat.h> #include <poll.h> #include <unistd.h> -#include <wait.h> +#include <sys/wait.h> #include "virerror.h" #include "virlog.h" diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index d1087aa10cf8..8f7acba9800c 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -26,7 +26,7 @@ #include <fcntl.h> #include <sys/apparmor.h> #include <unistd.h> -#include <wait.h> +#include <sys/wait.h> #include "internal.h" -- 2.35.1
The returned packet can have less strict alignment (u_char) than the struct (ether_header) we are casting it to, so to avoid alignment issues just copy the header into the struct on the stack. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/nwfilter/nwfilter_learnipaddr.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index fb552bd1e60e..99bffdc4fbdb 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -384,7 +384,7 @@ learnIPAddressThread(void *arg) struct bpf_program fp; struct pcap_pkthdr header; const u_char *packet; - struct ether_header *ether_hdr; + struct ether_header ether_hdr; struct ether_vlan_header *vlan_hdr; virNWFilterIPAddrLearnReq *req = arg; uint32_t vmaddr = 0, bcastaddr = 0; @@ -506,13 +506,14 @@ learnIPAddressThread(void *arg) } if (header.len >= sizeof(struct ether_header)) { - ether_hdr = (struct ether_header*)packet; + /* Avoid alignment issues */ + memcpy(ðer_hdr, packet, sizeof(struct ether_header)); - switch (ntohs(ether_hdr->ether_type)) { + switch (ntohs(ether_hdr.ether_type)) { case ETHERTYPE_IP: ethHdrSize = sizeof(struct ether_header); - etherType = ntohs(ether_hdr->ether_type); + etherType = ntohs(ether_hdr.ether_type); break; case ETHERTYPE_VLAN: @@ -528,7 +529,7 @@ learnIPAddressThread(void *arg) continue; } - if (virMacAddrCmpRaw(&req->binding->mac, ether_hdr->ether_shost) == 0) { + if (virMacAddrCmpRaw(&req->binding->mac, ether_hdr.ether_shost) == 0) { /* packets from the VM */ if (etherType == ETHERTYPE_IP && @@ -568,9 +569,9 @@ learnIPAddressThread(void *arg) } } } else if (virMacAddrCmpRaw(&req->binding->mac, - ether_hdr->ether_dhost) == 0 || + ether_hdr.ether_dhost) == 0 || /* allow Broadcast replies from DHCP server */ - virMacAddrIsBroadcastRaw(ether_hdr->ether_dhost)) { + virMacAddrIsBroadcastRaw(ether_hdr.ether_dhost)) { /* packets to the VM */ if (etherType == ETHERTYPE_IP && (header.len >= ethHdrSize + -- 2.35.1
When libc uses a define to rewrite stat64 to stat our mocks do not work if they are chained because the symbol that we are looking up is being stringified and therefore preventing the stat64->stat expansion per C-preprocessor rules. One stringification macro is just enough to make it work. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/virmock.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/virmock.h b/tests/virmock.h index 5250e733a156..300ba17174d4 100644 --- a/tests/virmock.h +++ b/tests/virmock.h @@ -296,12 +296,14 @@ do {} while (0) #endif +#define VIR_MOCK_STRINGIFY_SYMBOL(name) #name + #define VIR_MOCK_REAL_INIT(name) \ do { \ VIR_MOCK_REAL_INIT_MAIN(name, #name); \ if (real_##name == NULL && \ !(real_##name = dlsym(RTLD_NEXT, \ - #name))) { \ + VIR_MOCK_STRINGIFY_SYMBOL(name)))) { \ fprintf(stderr, "Missing symbol '" #name "'\n"); \ abort(); \ } \ -- 2.35.1
On Mon, Mar 07, 2022 at 10:04:06AM +0100, Martin Kletzander wrote:
When libc uses a define to rewrite stat64 to stat our mocks do not work if they are chained because the symbol that we are looking up is being stringified and therefore preventing the stat64->stat expansion per C-preprocessor rules. One stringification macro is just enough to make it work.
This doesn't sound right to me. If we're implementing a mock for 'stat64', we should not be looking up 'stat'. We should be implmenting a mock for 'stat' instead. It sounds more like we got MOCK_STAT and MOCK_STAT64 incorrectly defined surely. 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 Mon, Mar 07, 2022 at 10:01:40AM +0000, Daniel P. Berrangé wrote:
On Mon, Mar 07, 2022 at 10:04:06AM +0100, Martin Kletzander wrote:
When libc uses a define to rewrite stat64 to stat our mocks do not work if they are chained because the symbol that we are looking up is being stringified and therefore preventing the stat64->stat expansion per C-preprocessor rules. One stringification macro is just enough to make it work.
This doesn't sound right to me.
If we're implementing a mock for 'stat64', we should not be looking up 'stat'. We should be implmenting a mock for 'stat' instead.
It sounds more like we got MOCK_STAT and MOCK_STAT64 incorrectly defined surely.
I spent three days trying to figure this out. But with this fix we are actually doing what you are suggesting. All the (l)stat64 are being replaced by (l)stat, so even: int lstat64(const char *path, struct stat64 *statbuf) is actually rewritten to int lstat(const char *path, struct stat *statbuf) because the define applies to the struct name as well. The only difference is that internally it will use the real_lstat64 function pointer, but the function declaration is correct. And if you enable debugging it will say that lstat64 is being redirected, but that's a pretty minor issue, if it is one at all. What we could do is rewrite the macros so that all of the above is properly expanded/substituted, but I do not think that is the way we want to do things as it looked a bit messy (I probably still have that commit somewhere.
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 Mon, Mar 07, 2022 at 11:43:05AM +0100, Martin Kletzander wrote:
On Mon, Mar 07, 2022 at 10:01:40AM +0000, Daniel P. Berrangé wrote:
On Mon, Mar 07, 2022 at 10:04:06AM +0100, Martin Kletzander wrote:
When libc uses a define to rewrite stat64 to stat our mocks do not work if they are chained because the symbol that we are looking up is being stringified and therefore preventing the stat64->stat expansion per C-preprocessor rules. One stringification macro is just enough to make it work.
This doesn't sound right to me.
If we're implementing a mock for 'stat64', we should not be looking up 'stat'. We should be implmenting a mock for 'stat' instead.
It sounds more like we got MOCK_STAT and MOCK_STAT64 incorrectly defined surely.
I spent three days trying to figure this out. But with this fix we are actually doing what you are suggesting. All the (l)stat64 are being replaced by (l)stat, so even:
int lstat64(const char *path, struct stat64 *statbuf)
is actually rewritten to
int lstat(const char *path, struct stat *statbuf)
because the define applies to the struct name as well. The only difference is that internally it will use the real_lstat64 function pointer, but the function declaration is correct. And if you enable debugging it will say that lstat64 is being redirected, but that's a pretty minor issue, if it is one at all.
What we could do is rewrite the macros so that all of the above is properly expanded/substituted, but I do not think that is the way we want to do things as it looked a bit messy (I probably still have that commit somewhere.
Ok, thanks for the explanation. 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 3/7/22 10:04, Martin Kletzander wrote:
Compiling libvirt on alpine showed a bunch of things that are potentially wrong, we just haven't noticed elsewhere.
Martin Kletzander (5): syntax-check: Rework mock-noinline to get all files at once Include poll.h instead of sys/poll.h Include sys/wait.h instead of wait.h nwfilter: Avoid memory alignment issues tests: Allow expansion of mocked stat symbols
build-aux/syntax-check.mk | 2 +- scripts/mock-noinline.py | 2 +- src/lxc/lxc_driver.c | 4 ++-- src/network/bridge_driver.c | 2 +- src/nwfilter/nwfilter_learnipaddr.c | 15 ++++++++------- src/openvz/openvz_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/security/security_apparmor.c | 2 +- src/vz/vz_driver.c | 2 +- tests/virmock.h | 4 +++- 10 files changed, 20 insertions(+), 17 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Apparently, I don't know how C preprocessor works. Michal
On a Monday in 2022, Martin Kletzander wrote:
Compiling libvirt on alpine showed a bunch of things that are potentially wrong, we just haven't noticed elsewhere.
Martin Kletzander (5): syntax-check: Rework mock-noinline to get all files at once Include poll.h instead of sys/poll.h Include sys/wait.h instead of wait.h nwfilter: Avoid memory alignment issues tests: Allow expansion of mocked stat symbols
build-aux/syntax-check.mk | 2 +- scripts/mock-noinline.py | 2 +- src/lxc/lxc_driver.c | 4 ++-- src/network/bridge_driver.c | 2 +- src/nwfilter/nwfilter_learnipaddr.c | 15 ++++++++------- src/openvz/openvz_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/security/security_apparmor.c | 2 +- src/vz/vz_driver.c | 2 +- tests/virmock.h | 4 +++- 10 files changed, 20 insertions(+), 17 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (4)
-
Daniel P. Berrangé -
Ján Tomko -
Martin Kletzander -
Michal Prívozník