From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=VQS4oyhF; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id EAF715A004C for ; Fri, 25 Oct 2024 09:53:18 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1729842798; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=j9Z6GcsZ5YsiJflKMPIQ6lPKLyf2F0MQWuTevbhZ4hk=; b=VQS4oyhF4VTSw0WHcp6QYyopXDbrGZiB0nYSApjv9nY3GbGCpXWBOLhKkZhVA7qHk97/4n LO5NpfrzlLkrCigIDIdLuhhYihAUVj8eTmE0HJBhg5IFINt/YyYP7MYehq7t/qTdX2hEiN xlOX0SUWiIH/O6iJLLBqzNujTHQNDYE= Received: from mail-pg1-f197.google.com (mail-pg1-f197.google.com [209.85.215.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-379-1ttR67X9NpuWrAwOQzIEKQ-1; Fri, 25 Oct 2024 03:53:15 -0400 X-MC-Unique: 1ttR67X9NpuWrAwOQzIEKQ-1 Received: by mail-pg1-f197.google.com with SMTP id 41be03b00d2f7-7ea0dbb7cc1so2159247a12.0 for ; Fri, 25 Oct 2024 00:53:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729842792; x=1730447592; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=j9Z6GcsZ5YsiJflKMPIQ6lPKLyf2F0MQWuTevbhZ4hk=; b=JJrO233W1bNt9I0waNxkhAXI/mpJq+KGI+fU7LahDzfnsLhkQ9ujxkhIl/EjaZp0jV 7fexwvsWHaT1uaS17Vc6FZU1nn6Z8snBF+TFr9JwCcqgIzRQviJoaWK0bP7V7Wh59ZAg e77uTd8237PkG+paD1mp4hbQO8EZwb5UW5wQz6Hdv83cKg4fC0s5QcI4aMFpaTCO8SDX 2tgJZ7+Q7Bu6h2Jua2Rv5QnylEUCnPz1r5+xxiCMEmkV+N0yfYolIGfZftu86sqeDPim E1er4ZIefaDZ1xve6WjlvzbHjFBeYEmoDzKI5irW/exwe5YQKDBdHucCWs91jXdnfzWW eOeg== X-Gm-Message-State: AOJu0YwCns9cmNxc46wJicRZkFF7TWrdAMYGRwEO16I2zh1VurfdeJfL mv6n+SOu90aA/LUYjZ0OWuSUE0GHXaSIJFqVFF7qd6KMgL3ntV1OtfdF2SsiFVU7pRlOLxpvxxV O0e4gw1HXMysCnPVcneIuUlsMtZFEoDWurIsJdKkZXAD/P/e/Gz+vo4jdMg== X-Received: by 2002:a17:90b:3010:b0:2e0:89f2:f60c with SMTP id 98e67ed59e1d1-2e77e5fa1b0mr7616847a91.11.1729842791725; Fri, 25 Oct 2024 00:53:11 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFyjBN0BqhoreMUV75tUWWW2wWUWbTxQT075I6dO+Og410QOUYh9zqQurk4ENzpvvSXlNTLkA== X-Received: by 2002:a17:90b:3010:b0:2e0:89f2:f60c with SMTP id 98e67ed59e1d1-2e77e5fa1b0mr7616811a91.11.1729842790908; Fri, 25 Oct 2024 00:53:10 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2e77e59d826sm2763996a91.52.2024.10.25.00.53.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Oct 2024 00:53:06 -0700 (PDT) Date: Fri, 25 Oct 2024 09:53:03 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 2/8] treewide: Comply with CERT C rule ERR33-C for snprintf() Message-ID: <20241025095303.7f67060f@elisabeth> In-Reply-To: References: <20241024230438.3192725-1-sbrivio@redhat.com> <20241024230438.3192725-3-sbrivio@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 2LVHUSEC3YT4YKIIZJPKFT3QHC3OEZFN X-Message-ID-Hash: 2LVHUSEC3YT4YKIIZJPKFT3QHC3OEZFN X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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