On Fri, 17 Feb 2023 09:53:28 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  On Thu, Feb 16, 2023 at 07:22:10PM +0100, Stefano
Brivio wrote:
  The newly introduced die() calls exit(), but
cppcheck doesn't see it
 and warns about possibly invalid arguments used after the check which
 triggers die(). Add return statements to silence the warnings.
 
 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>   
 
 Oof, that's super ugly.  Any chance that cppcheck will recognize the
 ((noreturn)) attribute if we added it to die()? 
It doesn't. I guess Library::isnoreturn() in lib/library.cpp is fooled
by the way we build die with a macro. I couldn't find a corresponding
ticket, there's a vaguely related false _negative_ here:
	https://trac.cppcheck.net/ticket/7933

but I admit I might have missed one from this list:
	https://trac.cppcheck.net/query?status=!closed&keywords=~valueflow

I see a few alternatives:

- move exit() after the function body, instead of using 'doexit', I
  couldn't find a "nice" way to do so but it should be possible

- fix the issue in cppcheck

- or... would you prefer if I use a cppcheck-suppress token here?

-- 
Stefano

On 2/16/23 5:53 PM, David Gibson wrote:
...
  On Thu, Feb 16, 2023 at 07:22:10PM +0100, Stefano
Brivio wrote:
  The newly introduced die() calls exit(), but
cppcheck doesn't see it
 and warns about possibly invalid arguments used after the check which
 triggers die(). Add return statements to silence the warnings.

 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> 
 
 Oof, that's super ugly.  Any chance that cppcheck will recognize the
 ((noreturn)) attribute if we added it to die()? 
Why is this only a problem in these two files?

(and is there a "make check" target that I should have been running and 
haven't?)

Requiring an extra "return" after die() kind of removes the advantage of 
using it over err(). :-/ If we have to do that, it would be more 
straightforward to just use err() followed by exit() directly.

...
  
  ---
   conf.c | 3 +++
   tap.c  | 6 +++++-
   2 files changed, 8 insertions(+), 1 deletion(-)

 diff --git a/conf.c b/conf.c
 index 675d961..5426c9b 100644
 --- a/conf.c
 +++ b/conf.c
 @@ -1036,6 +1036,9 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
   	if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0) {
   		die("Can't determine if we're in init namespace: %s",
   		    strerror(errno));
 +
 +		/* Silence cppcheck's invalidFunctionArg for 'fd' in read() */
 +		return;
   	}
   
   	if (read(fd, buf, BUFSIZ) != sizeof(root_uid_map) ||
 diff --git a/tap.c b/tap.c
 index 88eed88..d6f962e 100644
 --- a/tap.c
 +++ b/tap.c
 @@ -1037,9 +1037,13 @@ static void tap_sock_unix_init(struct ctx *c)
   			snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
   
   		ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
 -		if (ex < 0)
 +		if (ex < 0) {
   			die("UNIX domain socket check: %s", strerror(errno));
   
 +			/* Silence cppcheck's invalidFunctionArg for 'ex' */
 +			return;
 +		}
 +
   		ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr));
   		if (!ret || (errno != ENOENT && errno != ECONNREFUSED &&
   			     errno != EACCES)) {