[PATCH ucspi-vsock 0/2] Fix clang-tidy warnings
clang-tidy also produced this warning: /home/src/ucspi-vsock/repro.c:43:19: warning: unused parameter 'sig' [clang-diagnostic-unused-parameter] void sig_exit(int sig) { exit(EX_UNAVAILABLE); } ^ vsock.c:27:23: warning: The left operand of '!=' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult] if (addr->svm_family != AF_VSOCK) { ^ vsock.c:90:6: note: Assuming the condition is false if (getsockname(fd, (struct sockaddr *)&addr, &addrlen) == -1) ^ vsock.c:90:2: note: Taking false branch if (getsockname(fd, (struct sockaddr *)&addr, &addrlen) == -1) ^ vsock.c:93:9: note: Calling 'fill_cid_and_port' return fill_cid_and_port(&addr, cid, port); ^ vsock.c:27:23: note: The left operand of '!=' is a garbage value if (addr->svm_family != AF_VSOCK) { But I think this is just warning me that the POSIX socket API violates the strict aliasing rule. (Which is true, but there's not a lot I can do about it...) It also tells me I should use memset_s instead of memset, but, well... https://en.wikipedia.org/wiki/C11_(C_standard_revision)#Criticism Alyssa Ross (2): exec: free argv if exec fails vsockserver: fix uninitialized variable exec.c | 5 ++++- vsockserver.c | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) -- 2.30.0
Identified by clang-tidy: exec.c:21:2: warning: Potential leak of memory pointed to by 'argv' [clang-analyzer-unix.Malloc] return execvp(file, argv); ^ exec.c:15:16: note: Memory is allocated char **argv = calloc(argz_count(argz, len) + 1, sizeof(char *)); ^ exec.c:16:6: note: Assuming 'argv' is non-null if (!argv) ^ exec.c:16:2: note: Taking false branch if (!argv) ^ exec.c:21:2: note: Potential leak of memory pointed to by 'argv' return execvp(file, argv); --- I'm not used to thinking about allocations around exec, since usually if it fails the program just terminates. But in a function like this, the rules are a bit different. exec.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/exec.c b/exec.c index cc55200..75b1980 100644 --- a/exec.c +++ b/exec.c @@ -18,5 +18,8 @@ int execzp(const char *file, const char *argz, size_t len) argz_extract(argz, len, argv); - return execvp(file, argv); + execvp(file, argv); + + free(argv); + return -1; } -- 2.30.0
Identified by clang-tidy: vsockserver.c:105:17: warning: 3rd function call argument is an uninitialized value [clang-analyzer-core.CallAndMessage] while ((opt = argz_next(binder_opts, binder_opts_len, opt))) ^ vsockserver.c:44:3: note: Assuming the condition is false argz_add(&binder_opts, &binder_opts_len, BINDIR "/vsockserver-socketbinder") || ^ vsockserver.c:44:3: note: Left side of '||' is false vsockserver.c:47:9: note: Assuming the condition is false while ((opt = getopt(argc, argv, "+1qQv")) != -1) { ^ vsockserver.c:47:2: note: Loop condition is false. Execution continues on line 68 while ((opt = getopt(argc, argv, "+1qQv")) != -1) { ^ vsockserver.c:68:6: note: Assuming 'alloc_failed' is false if (alloc_failed) ^ vsockserver.c:68:2: note: Taking false branch if (alloc_failed) ^ vsockserver.c:77:6: note: Assuming the condition is false if (optind > argc - 3) ^ vsockserver.c:77:2: note: Taking false branch if (optind > argc - 3) ^ vsockserver.c:81:6: note: Assuming the condition is false if (argz_add(&binder_opts, &binder_opts_len, "--") || ^ vsockserver.c:81:6: note: Left side of '||' is false vsockserver.c:82:6: note: Assuming the condition is false argz_add(&binder_opts, &binder_opts_len, argv[optind++]) || ^ vsockserver.c:81:6: note: Left side of '||' is false if (argz_add(&binder_opts, &binder_opts_len, "--") || ^ vsockserver.c:83:6: note: Assuming the condition is false argz_add(&binder_opts, &binder_opts_len, argv[optind++])) ^ vsockserver.c:81:2: note: Taking false branch if (argz_add(&binder_opts, &binder_opts_len, "--") || ^ vsockserver.c:89:6: note: Assuming the condition is false if (argz_append(&binder_opts, &binder_opts_len, daemon_opts, daemon_opts_len)) ^ vsockserver.c:89:2: note: Taking false branch if (argz_append(&binder_opts, &binder_opts_len, daemon_opts, daemon_opts_len)) ^ vsockserver.c:96:23: note: Assuming 'i' is >= 'argc' for (int i = optind; i < argc; i++) ^ vsockserver.c:96:2: note: Loop condition is false. Execution continues on line 100 for (int i = optind; i < argc; i++) ^ vsockserver.c:100:6: note: Assuming 'verbosity' is equal to all if (verbosity == all) { ^ vsockserver.c:100:2: note: Taking true branch if (verbosity == all) { ^ vsockserver.c:101:3: note: 'opt' declared without an initial value char *opt; ^ vsockserver.c:105:17: note: 3rd function call argument is an uninitialized value while ((opt = argz_next(binder_opts, binder_opts_len, opt))) --- vsockserver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vsockserver.c b/vsockserver.c index f740a8a..b717eee 100644 --- a/vsockserver.c +++ b/vsockserver.c @@ -98,7 +98,7 @@ int main(int argc, char *argv[]) diee(EX_OSERR, "malloc"); if (verbosity == all) { - char *opt; + char *opt = 0; // Log the full argv before we exec it. fprintf(stderr, "%s: executing", program_invocation_short_name); -- 2.30.0
On Sun Mar 21, 2021 at 7:46 AM PDT, Alyssa Ross wrote:
clang-tidy also produced this warning:
/home/src/ucspi-vsock/repro.c:43:19: warning: unused parameter 'sig' [clang-diagnostic-unused-parameter] void sig_exit(int sig) { exit(EX_UNAVAILABLE); } ^ vsock.c:27:23: warning: The left operand of '!=' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult] if (addr->svm_family != AF_VSOCK) { ^ vsock.c:90:6: note: Assuming the condition is false if (getsockname(fd, (struct sockaddr *)&addr, &addrlen) == -1) ^ vsock.c:90:2: note: Taking false branch if (getsockname(fd, (struct sockaddr *)&addr, &addrlen) == -1) ^ vsock.c:93:9: note: Calling 'fill_cid_and_port' return fill_cid_and_port(&addr, cid, port); ^ vsock.c:27:23: note: The left operand of '!=' is a garbage value if (addr->svm_family != AF_VSOCK) {
But I think this is just warning me that the POSIX socket API violates the strict aliasing rule. (Which is true, but there's not a lot I can do about it...)
It also tells me I should use memset_s instead of memset, but, well... https://en.wikipedia.org/wiki/C11_(C_standard_revision)#Criticism
Alyssa Ross (2): exec: free argv if exec fails vsockserver: fix uninitialized variable
exec.c | 5 ++++- vsockserver.c | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-)
-- 2.30.0
Patchset LGTM.
Reviewed-by: Cole Helbling
On Sun, Mar 21, 2021 at 09:30:42AM -0700, Cole Helbling wrote:
On Sun Mar 21, 2021 at 7:46 AM PDT, Alyssa Ross wrote:
clang-tidy also produced this warning:
/home/src/ucspi-vsock/repro.c:43:19: warning: unused parameter 'sig' [clang-diagnostic-unused-parameter] void sig_exit(int sig) { exit(EX_UNAVAILABLE); } ^ vsock.c:27:23: warning: The left operand of '!=' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult] if (addr->svm_family != AF_VSOCK) { ^ vsock.c:90:6: note: Assuming the condition is false if (getsockname(fd, (struct sockaddr *)&addr, &addrlen) == -1) ^ vsock.c:90:2: note: Taking false branch if (getsockname(fd, (struct sockaddr *)&addr, &addrlen) == -1) ^ vsock.c:93:9: note: Calling 'fill_cid_and_port' return fill_cid_and_port(&addr, cid, port); ^ vsock.c:27:23: note: The left operand of '!=' is a garbage value if (addr->svm_family != AF_VSOCK) {
But I think this is just warning me that the POSIX socket API violates the strict aliasing rule. (Which is true, but there's not a lot I can do about it...)
It also tells me I should use memset_s instead of memset, but, well... https://en.wikipedia.org/wiki/C11_(C_standard_revision)#Criticism
Alyssa Ross (2): exec: free argv if exec fails vsockserver: fix uninitialized variable
exec.c | 5 ++++- vsockserver.c | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-)
-- 2.30.0
Patchset LGTM.
Reviewed-by: Cole Helbling
Thanks! To ssh://atuin/home/spectrum/git/ucspi-vsock.git 20a27f1..8b690b9 8b690b9 -> master
(I should really make a macro or something for that ^, so I don't have to type it out every time and potentially make a mistake :D)
participants (2)
-
Alyssa Ross
-
Cole Helbling