[PATCH] conf, tap: Silence two false positive invalidFunctionArg from cppcheck

Stefano Brivio

16 Feb 2023 16 Feb '23

7:22 p.m.

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 --- 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)) { -- 2.35.1

Show replies by date

David Gibson

16 Feb 16 Feb

11:53 p.m.

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

Oof, that's super ugly. Any chance that cppcheck will recognize the ((noreturn)) attribute if we added it to die()?

...

--- 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)) {

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Stefano Brivio

17 Feb 17 Feb

9:04 a.m.

On Fri, 17 Feb 2023 09:53:28 +1100 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

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

David Gibson

10:10 a.m.

On Fri, Feb 17, 2023 at 09:04:53AM +0100, Stefano Brivio wrote:

...

On Fri, 17 Feb 2023 09:53:28 +1100 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

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

I prefer this option - I don't love flags which dramatically change behaviour any way.

...

- fix the issue in cppcheck

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

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Laine Stump

3:29 p.m.

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

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)) {

Stefano Brivio

3:37 p.m.

On Fri, 17 Feb 2023 09:29:51 -0500 Laine Stump wrote:

...

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

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?

Because that's where we would use a variable as a system call argument, if the check possibly leading to die() didn't exist.

...

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

make cppcheck, make clang-tidy, and possibly the tests (see test/README.md) -- but it's not mandatory, especially if you're an occasional contributor, I'm running them anyway.

...

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.

We don't have to (see the rest of this thread): we could simply define die() as err() with an additional return statement *outside* err() itself, instead of passing 'doexit' as parameter. Probably something like: do { err(...) exit(EXIT_FAILURE); return; } while (0) -- Stefano

838

Age (days ago)

839

Last active (days ago)

List overview

Download

5 comments

3 participants

participants (3)

David Gibson
Laine Stump
Stefano Brivio