[PATCH v2 00/28] Fixes for static checkers
The passt tests include two static checking tools: clang-tidy and cppcheck. However, newer versions of those tools have introduced extra checks, and may cause these tests to fail. This series fixes all the clang-tidy and cppcheck warnings, either by altering our code, or by suppressing them with relevant options to the checkers. With this series, the checks are now clean on both my Fedora 36 machine (clang-tools-extra-14.0.5-1.fc36.x86_64 and cppcheck-2.7.4-2.fc36.x86_64) and my Debian machine (bookworm with some pieces from sid: clang-tidy 1:14.0-55.1 and cppcheck 2.9-1). Changes since v1: * Fixed a whitespace error * Added extra background information and details to comments and commit messages when removing old suppressions * Improved conf_runas() rework to give better error messages David Gibson (28): Clean up parsing of port ranges clang-tidy: Suppress warning about unchecked error in logfn macro clang-tidy: Fix spurious null pointer warning in pasta_start_ns() clang-tidy: Remove duplicate #include from icmp.c Catch failures when installing signal handlers Pack DHCPv6 "on wire" structures Clean up parsing in conf_runas() cppcheck: Reduce scope of some variables Don't shadow 'i' in conf_ports() Don't shadow global function names Stricter checking for nsholder.c cppcheck: Work around false positive NULL pointer dereference error cppcheck: Use inline suppression for ffsl() cppcheck: Use inline suppressions for qrap.c cppcheck: Use inline suppression for strtok() in conf.c Avoid ugly 'end' members in netlink structures cppcheck: Broaden suppression for unused struct members cppcheck: Remove localtime suppression for pcap.c qrap: Handle case of PATH environment variable being unset cppcheck: Suppress same-value-in-ternary branches warning cppcheck: Suppress NULL pointer warning in tcp_sock_consume() Regenerate seccomp.h if seccomp.sh changes cppcheck: Avoid errors due to zeroes in bitwise ORs cppcheck: Remove unused knownConditionTrueFalse suppression cppcheck: Remove unused objectIndex suppressions cppcheck: Remove unused va_list_usedBeforeStarted suppression Mark unused functions for cppcheck cppcheck: Remove unused unmatchedSuppression suppressions Makefile | 26 +--- arch.c | 4 +- conf.c | 359 +++++++++++++++++++++++------------------------- dhcpv6.c | 26 ++-- icmp.c | 1 - igmp.c | 1 + netlink.c | 22 +-- passt.c | 7 +- pasta.c | 5 +- qrap.c | 18 ++- seccomp.sh | 2 + siphash.c | 1 + tap.c | 5 +- tcp.c | 5 + test/Makefile | 2 +- test/nsholder.c | 2 +- util.c | 2 +- util.h | 1 + 18 files changed, 236 insertions(+), 253 deletions(-) -- 2.37.3
conf_ports() parses ranges of ports for the -t, -u, -T and -U options.
The code is quite difficult to the follow, to the point that clang-tidy
and cppcheck disagree on whether one of the pointers can be NULL at some
points.
Rework the code with the use of two new helper functions:
* parse_port_range() operates a bit like strtoul(), but can parse a whole
port range specification (e.g. '80' or '1000-1015')
* next_chunk() does the necessary wrapping around strchr() to advance to
just after the next given delimiter, while cleanly handling if there
are no more delimiters
The new version is easier to follow, and also removes some cppcheck
warnings.
Signed-off-by: David Gibson
clang-tidy complains that we're not checking the result of vfprintf in
logfn(). There's not really anything we can do if this fails here, so just
suppress the error with a cast to void.
Signed-off-by: David Gibson
clang-tidy isn't quite clever enough to figure out that getenv("SHELL")
will return the same thing both times here, which makes it conclude that
shell could be NULL, causing problems later.
It's a bit ugly that we call getenv() twice in any case, so rework this in
a way that clang-tidy can figure out shell won't be NULL.
Signed-off-by: David Gibson
Signed-off-by: David Gibson
Stop ignoring the return codes from sigaction() and signal(). Unlikely to
happen in practice, but if it ever did it could lead to really hard to
debug problems. So, take clang-tidy's advice and check for errors here.
Signed-off-by: David Gibson
dhcpv6.c contains a number of structures which represent actual DHCPv6
packets as they appear on the wire, which will break if the structures
don't have exactly the in-memory layout we expect.
Therefore, we should mark these structures as ((packed)). The contents of
them means this is unlikely to change the layout in practice - and since
it was working, presumably didn't on any arch we were testing on. However
it's not impossible for the compiler on some arch to insert unexpected
padding in one of these structures, so we should be explicit.
clang-tidy warned about this since we were using memcmp() to compare some
of these structures, which it thought might not have a unique
representation.
Signed-off-by: David Gibson
conf_runas() handles several of the different possible cases for the
--runas argument in a slightly odd order. Although it can parse both
numeric UIDs/GIDs and user/group names, it can't parse a numeric UID
combined with a group name or vice versa. That's not obviously useful, but
it's slightly surprising gap to have.
Rework the parsing to be more systematic: first split the option into
user and (optional) group parts, then separately parse each part as either
numeric or a name. As a bonus this removes some clang-tidy warnings.
While we're there also add cppcheck suppressions for getpwnam() and
getgrnam(). It complains about those because they're not reentrant.
passt is single threaded though, and is always likely to be during
this initialization code, even if we multithread later.
There were some existing suppressions for these in the cppcheck
invocation but they're no longer up to date. Replace them with inline
suppressions which, being next to the code, are more likely to stay
correct.
Signed-off-by: David Gibson
Minor style improvement suggested by cppcheck.
Signed-off-by: David Gibson
The counter 'i' is used in a number of places in conf_ports(), but in one
of those we unnecessarily shadow it in an inner scope. We could re-use the
same 'i' every time, but each use is logically separate, so instead remove
the outer declaration and declare it locally in each of the clauses where
we need it.
While we're there change it from a signed to unsigned int, since it's used
to iterate over port numbers which are generally treated as unsigned.
Signed-off-by: David Gibson
cppcheck points out that qrap's main shadows the global err() function with
a local. Rename it to rc to avoid the clash.
Signed-off-by: David Gibson
Add the -Wextra -pedantic and -std=c99 flags when compiling the nsholder
test helper to get extra compiler checks, like we already use for the
main source code.
While we're there, fix some %d (signed) printf descriptors being used
for unsigned values (uid_t and gid_t). Pointed out by cppcheck.
Signed-off-by: David Gibson
Some versions of cppcheck could errneously report a NULL pointer deference
inside a sizeof(). This is now fixed in cppcheck upstream[0]. For systems
using an affected version, add a suppression to work around the bug. Also
add an unmatchedSuppression suppression so the suppression itself doesn't
cause a warning if you *do* have a fixed cppcheck.
[0] https://github.com/danmar/cppcheck/pull/4471
Signed-off-by: David Gibson
We define our own ffsl() as a weak symbol, in case our C library doesn't
include it. On glibc systems which *do* include it, this causes a cppcheck
warning because unsurprisingly our version doesn't pick the same argument
names. Convert the suppression for this into an inline suppression.
Signed-off-by: David Gibson
qrap.c uses several old-fashioned functions that cppcheck complains about.
Since it's headed for obselesence anyway, just suppress these rather than
attempting to modernize the code.
Signed-off-by: David Gibson
strtok() is non-reentrant and old-fashioned, so cppcheck would complains
about its use in conf.c if it weren't suppressed. We're single threaded
and strtok() is convenient though, so it's not really worth reworking at
this time. Convert this to an inline suppression so it's adjacent to the
code its annotating.
Signed-off-by: David Gibson
We use a number of complex structures to format messages to send to
netlink. In some cases we add imaginary 'end' members not because they
actually mean something on the wire, but so that we can use offsetof() on
the member to determine the relevant size.
Adding extra things to the structures for this is kinda nasty. We can use
a different construct with offsetof and sizeof to avoid them. As a bonus
this removes some cppcheck warnings about unused struct members.
Signed-off-by: David Gibson
In a number of places in passt we use structures to represent over the wire
or in-file data with a fixed layout. After initialization we don't access
the fields individually and just write the structure as a whole to its
destination.
Unfortunately cppcheck doesn't cope with this pattern and thinks all the
structure members are unused. We already have suppressions for this in
pcap.c and dhcp.c However, it also appears in dhcp.c and netlink.c at
least. Since this is likely to be common, it seems wiser to just suppress
the error globally.
Signed-off-by: David Gibson
Since bf95322f "conf: Make the argument to --pcap option mandatory" we
no longer call localtime() in pcap.c, so we no longer need the matching
cppcheck suppression.
Signed-off-by: David Gibson
clang-tidy warns that in passing getenv("PATH") to strncpy() we could be
passing a NULL pointer. While it's unusual for PATH to be unset, it's not
impossible and this would indeed cause getenv() to return NULL.
Handle this case by never recognizing argv[2] as a qemu binary name if
PATH is not set. This is... no flakier than the detection of whether
it's a binary name already is.
Signed-off-by: David Gibson
TIMER_INTERVAL is the minimum of two separately defined intervals which
happen to have the same value at present. This results in an expression
which has the same value in both branches of a ternary operator, which
cppcheck warngs about. This is logically sound in this case, so suppress
the error (we appear to already have a similar suppression for clang-tidy).
Also add an unmatchedSuppression suppression, since only some cppcheck
versions complain about this instance.
Signed-off-by: David Gibson
Recent versions of cppcheck give a warning due to the NULL buffer passed
to recv() in tcp_sock_consume(). In fact this is safe, because we're using
MSG_TRUNC which means the kernel will ignore the buffer. Use a suppression
to get rid of the error. Also add an unmatchedSuppression suppression since
only some cppcheck versions complain about this.
Signed-off-by: David Gibson
seccomp.sh generates seccomp.h, so if we change it, we should re-build
seccomp.h as well. Add this to the make dependencies so it happens.
Signed-off-by: David Gibson
Recent versions of cppcheck give warnings if using a bitwise OR (|) where
some of the arguments are zero. We're triggering these warnings in our
generated seccomp.h header, because BPF_LD and BPF_W are zero-valued.
However putting these defines in makes the generate code clearer, even
though they could be left out without changing the values. So, add
cppcheck suppressions to the generated code.
Signed-off-by: David Gibson
I can't get this warning to trigger, so I think this suppression must be
out of date. Whether that's because we've changed our code to no longer
have the problem, or because cppcheck itself has been updated to remove a
false positive I don't know.
If we find that we do need a suppression like this for some cppcheck
version, we should replace it with an inline suppression so it's clear
what exactly is triggering the warning.
Signed-off-by: David Gibson
These warnings no longer on cppcheck. The one in tcp.c was removed by
38fbfdbcb916 ("tcp: Get rid of iov with cached MSS, drop sendmmsg(),
add deferred flush"), I'm not sure exactly where the udp.c one was
fixed (or if it was an unneeded suppression in the first place), but
it no longer seems to be needed in any case.
Signed-off-by: David Gibson
I can't get this warning to trigger, even without the suppression, so
remove it. If it shows up again on some cppcheck version, we can replace
it with inline suppressions so it's clear where the issue lay.
Signed-off-by: David Gibson
We have a couple of functions that are unused (for now) by design.
Although at least one has a flag so that gcc doesn't warn, cppcheck has its
own warnings about this. Add specific inline suppressions for these rather
than a blanket suppression in the Makefile.
Signed-off-by: David Gibson
It's unclear what original suppressions these unmatchedSuppression
suppressions were supposed to go with. They don't trigger any warnings on
the current code that I can tell, so remove them. If we find a problem
with some cppcheck versions in future, replace them with inline
suppressions so it's clearer exactly where the issue is originating.
Signed-off-by: David Gibson
On Thu, 29 Sep 2022 13:38:04 +1000
David Gibson
The passt tests include two static checking tools: clang-tidy and cppcheck. However, newer versions of those tools have introduced extra checks, and may cause these tests to fail.
This series fixes all the clang-tidy and cppcheck warnings, either by altering our code, or by suppressing them with relevant options to the checkers. With this series, the checks are now clean on both my Fedora 36 machine (clang-tools-extra-14.0.5-1.fc36.x86_64 and cppcheck-2.7.4-2.fc36.x86_64) and my Debian machine (bookworm with some pieces from sid: clang-tidy 1:14.0-55.1 and cppcheck 2.9-1).
Changes since v1: * Fixed a whitespace error * Added extra background information and details to comments and commit messages when removing old suppressions * Improved conf_runas() rework to give better error messages
Applied, together with your previous batch (#7, v2) of test fixes. -- Stefano
participants (2)
-
David Gibson
-
Stefano Brivio