Re: [PATCH 2/8] treewide: Comply with CERT C rule ERR33-C for snprintf()

On Fri, 25 Oct 2024 11:48:55 +1100 David Gibson wrote:

On Fri, Oct 25, 2024 at 01:04:32AM +0200, Stefano Brivio wrote:

...

clang-tidy, starting from LLVM version 16, up to at least LLVM version 19, now checks that we detect and handle errors for snprintf() as requested by CERT C rule ERR33-C. These warnings were logged with LLVM version 19.1.2 (at least Debian and Fedora match):

/home/sbrivio/passt/arch.c:43:3: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors] 43 | snprintf(new_path, PATH_MAX + sizeof(".avx2"), "%s.avx2", exe); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/sbrivio/passt/arch.c:43:3: note: cast the expression to void to silence this warning /home/sbrivio/passt/conf.c:577:4: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors] 577 | snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/sbrivio/passt/conf.c:577:4: note: cast the expression to void to silence this warning /home/sbrivio/passt/conf.c:579:5: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors] 579 | snprintf(userns, PATH_MAX, "/proc/%ld/ns/user", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 580 | pidval); | ~~~~~~~ /home/sbrivio/passt/conf.c:579:5: note: cast the expression to void to silence this warning /home/sbrivio/passt/pasta.c:105:2: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors] 105 | snprintf(ns, PATH_MAX, "/proc/%i/ns/net", pasta_child_pid); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/sbrivio/passt/pasta.c:105:2: note: cast the expression to void to silence this warning /home/sbrivio/passt/pasta.c:242:2: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors] 242 | snprintf(uidmap, BUFSIZ, "0 %u 1", uid); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/sbrivio/passt/pasta.c:242:2: note: cast the expression to void to silence this warning /home/sbrivio/passt/pasta.c:243:2: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors] 243 | snprintf(gidmap, BUFSIZ, "0 %u 1", gid); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/sbrivio/passt/pasta.c:243:2: note: cast the expression to void to silence this warning /home/sbrivio/passt/tap.c:1155:4: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [cert-err33-c,-warnings-as-errors] 1155 | snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/sbrivio/passt/tap.c:1155:4: note: cast the expression to void to silence this warning

Don't silence the warnings as they might actually have some merit. Add an snprintf_check() function, instead, checking that we're not truncating messages while printing to buffers and terminating if we do.

Huh, I wonder why I wasn't seeing these with clang 18.1.8.1. Still, LGTM except for a couple of nits.

Somewhat interestingly, I also don't see those on Fedora 40 with LLVM 18.1.6, but I see them on Fedora Rawhide with LLVM 19, and on Debian testing with both LLVM 16 and LLVM 19.

...

Signed-off-by: Stefano Brivio --- arch.c | 4 +++- conf.c | 11 +++++++---- pasta.c | 7 ++++--- tap.c | 8 +++++--- util.c | 22 ++++++++++++++++++++++ util.h | 3 +++ 6 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/arch.c b/arch.c index 04bebfc..edbe666 100644 --- a/arch.c +++ b/arch.c @@ -19,6 +19,7 @@ #include

#include "log.h" +#include "util.h"

/** * arch_avx2_exec() - Switch to AVX2 build if supported @@ -40,7 +41,8 @@ void arch_avx2_exec(char **argv) if (__builtin_cpu_supports("avx2")) { char new_path[PATH_MAX + sizeof(".avx2")];

- snprintf(new_path, PATH_MAX + sizeof(".avx2"), "%s.avx2", exe); + snprintf_check("Can't build AVX2 executable path", new_path,

For all of these messages I'd suggest "XXX path is too long" rather than just "Can't build XXX path" in order to be more explicit and give users a clue to a workaround.

I originally wanted to do that, but snprintf() could return a negative value on "output error", which we should never get, but what if we have a C library interpreting that loosely and including conversion errors? Then we can't (practically) be more specific, unless we print messages in snprintf_check() itself, but you're suggesting we avoid doing that below, so I'd rather keep those as they are. We'll never print these anyway.

...

+ PATH_MAX + sizeof(".avx2"), "%s.avx2", exe); execve(new_path, argv, environ); warn_perror("Can't run AVX2 build, using non-AVX2 version"); } diff --git a/conf.c b/conf.c index b3b5342..2122600 100644 --- a/conf.c +++ b/conf.c @@ -574,10 +574,13 @@ static void conf_pasta_ns(int *netns_only, char *userns, char *netns, if (pidval < 0 || pidval > INT_MAX) die("Invalid PID %s", argv[optind]);

- snprintf(netns, PATH_MAX, "/proc/%ld/ns/net", pidval); - if (!*userns) - snprintf(userns, PATH_MAX, "/proc/%ld/ns/user", - pidval); + snprintf_check("Can't build netns path", netns, + PATH_MAX, "/proc/%ld/ns/net", pidval); + if (!*userns) { + snprintf_check("Can't build userns path", + userns, PATH_MAX, + "/proc/%ld/ns/user", pidval); + } } }

diff --git a/pasta.c b/pasta.c index 307fb4a..ce9ae7a 100644 --- a/pasta.c +++ b/pasta.c @@ -102,7 +102,8 @@ static int pasta_wait_for_ns(void *arg) int flags = O_RDONLY | O_CLOEXEC; char ns[PATH_MAX];

- snprintf(ns, PATH_MAX, "/proc/%i/ns/net", pasta_child_pid); + snprintf_check("Can't build netns path", ns, + PATH_MAX, "/proc/%i/ns/net", pasta_child_pid); do { while ((c->pasta_netns_fd = open(ns, flags)) < 0) { if (errno != ENOENT) @@ -239,8 +240,8 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, c->quiet = 1;

/* Configure user and group mappings */ - snprintf(uidmap, BUFSIZ, "0 %u 1", uid); - snprintf(gidmap, BUFSIZ, "0 %u 1", gid); + snprintf_check("Can't build uidmap", uidmap, BUFSIZ, "0 %u 1", uid); + snprintf_check("Can't build gidmap", gidmap, BUFSIZ, "0 %u 1", gid);

if (write_file("/proc/self/uid_map", uidmap) || write_file("/proc/self/setgroups", "deny") || diff --git a/tap.c b/tap.c index c53a39b..51eb134 100644 --- a/tap.c +++ b/tap.c @@ -1149,10 +1149,12 @@ int tap_sock_unix_open(char *sock_path) char *path = addr.sun_path; int ex, ret;

- if (*sock_path) + if (*sock_path) { memcpy(path, sock_path, UNIX_PATH_MAX); - else - snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i); + } else { + snprintf_check("Can't build UNIX domain path", path,

I'd suggest explicitly saying "Unix domain _socket_",

Oops, changed.

...

+ UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i); + }

ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0); if (ex < 0) diff --git a/util.c b/util.c index eba7d52..eff6787 100644 --- a/util.c +++ b/util.c @@ -749,3 +749,25 @@ void close_open_files(int argc, char **argv) if (rc) die_perror("Failed to close files leaked by parent"); } + +/** + * snprintf_check() - snprintf() wrapper, terminating on truncation + * @errmsg: Error string to be printed while terminating + * @str: Output buffer + * @size: Maximum size to write to @str + * @format: Message + */ +void snprintf_check(const char *errstr, + char *str, size_t size, const char *format, ...)

YMMV, but I always find passing a format / error string into a function that's not primarily about error handling kind of clunky. How much bulkier would it be to make this wrapper return a boolean and do an explicit: if (!sprintf_check(...)) die("blah blah blah");

at the call sites? It might make the control flow a little more obvious, and means we could add parameters to the error message if there are cases where that makes sense.

Right, yes. Changed, but with false (0) meaning success instead of failure, for consistency. -- Stefano