On 8/27/19 4:35 PM, Victor Toso wrote:
Hi,
Sorry, forgot to reply earlier.
On Tue, Aug 27, 2019 at 03:12:24PM +0300, Uri Lublin wrote:
On 8/27/19 1:27 PM, Frediano Ziglio wrote:
From: Victor Toso <me@xxxxxxxxxxxxxx>
Otherwise we get a CLANG_WARNING due accessing garbage.
Covscan report:
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:471:9: warning: 1st function
> call argument is an uninitialized value
> # execvp(orig_argv[0], orig_argv);
> # ^ ~~~~~~~~~~~~
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:421:24: note: Storing
> uninitialized value
> # char **orig_argv = g_memdup(argv, sizeof(char*) * (argc+1));
> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Here it doesn't say anything about g_memdup() itself (like, if it
took the branch where argv is NULL which means it returns NULL or
if it took the branch wehre argv is not NULL which it does
g_malloc + memcpy, same you suggested.
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:434:9: note: Assuming 'error'
> is equal to NULL
> # if (error != NULL) {
> # ^~~~~~~~~~~~~
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:434:5: note: Taking false
> branch
> # if (error != NULL) {
> # ^
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:442:9: note: Assuming 'portdev'
> is not equal to NULL
> # if (portdev == NULL)
> # ^~~~~~~~~~~~~~~
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:442:5: note: Taking false
> branch
> # if (portdev == NULL)
> # ^
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:445:9: note: Assuming
> 'vdagentd_socket' is not equal to NULL
> # if (vdagentd_socket == NULL)
> # ^~~~~~~~~~~~~~~~~~~~~~~
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:445:5: note: Taking false
> branch
> # if (vdagentd_socket == NULL)
> # ^
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:448:30: note: Assuming
> 'do_daemonize' is 0
> # openlog("spice-vdagent", do_daemonize ? LOG_PID : (LOG_PID |
> LOG_PERROR),
> # ^~~~~~~~~~~~
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:448:30: note: '?' condition is
> false
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:451:9: note: Assuming the
> condition is false
> # if (!g_file_test(portdev, G_FILE_TEST_EXISTS)) {
> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:451:5: note: Taking false
> branch
> # if (!g_file_test(portdev, G_FILE_TEST_EXISTS)) {
> # ^
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:457:9: note: Assuming
> 'do_daemonize' is 0
> # if (do_daemonize)
> # ^~~~~~~~~~~~
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:457:5: note: Taking false
> branch
> # if (do_daemonize)
> # ^
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:468:9: note: Assuming
> 'version_mismatch' is not equal to 0
> # if (version_mismatch) {
> # ^~~~~~~~~~~~~~~~
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:468:5: note: Taking true branch
> # if (version_mismatch) {
> # ^
> spice-vdagent-0.19.0/src/vdagent/vdagent.c:471:9: note: 1st function call
> argument is an uninitialized value
> # execvp(orig_argv[0], orig_argv);
> # ^ ~~~~~~~~~~~~
> # 469| syslog(LOG_INFO, "Version mismatch, restarting");
> # 470| sleep(1);
> # 471|-> execvp(orig_argv[0], orig_argv);
> # 472| }
> # 473|
Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
---
src/vdagent/vdagent.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
index 0e2e73e..982fc72 100644
--- a/src/vdagent/vdagent.c
+++ b/src/vdagent/vdagent.c
@@ -418,7 +418,11 @@ int main(int argc, char *argv[])
GOptionContext *context;
GError *error = NULL;
VDAgent *agent;
- char **orig_argv = g_memdup(argv, sizeof(char*) * (argc+1));
+ char **orig_argv;
+
+ g_return_val_if_fail(argc > 0 && argv != NULL, -1);
+ orig_argv = g_memdup(argv, sizeof(char*) * (argc+1));
Hi,
I was able to "fix" it by replacing g_memdup with g_malloc + memcpy
- char **orig_argv = g_memdup(argv, sizeof(char*) * (argc+1));
+ char **orig_argv = g_malloc(sizeof(char*) * (argc+1) );
+ memcpy(orig_argv, argv, sizeof(char*) * (argc+1) );
So clang seems to be confused by "side effects" of g_memdup.
I didn't test it but I trust you that it works. Weird that if
this complain was with the fact that orig_argv is NULL, it should
be more explicit. Doesn't matter much the possible fix as this
still is a false positive, argc is > 0 and argv is not null and
either g_memdup() is going to return valid memory or abort if
ENOMEM.
I think it does not complain about orig_argv being null but possibly
uninitialized. Maybe it has to do with a mixture of g_memdup and
argv which is not initialized in the program (but by the system).
My 'fix' is just because we really have an extra char* which
seems to be uninitialized.
As Frediano mentioned there is no need to check neither
argc>0 nor argv!=NULL , as both are always true.
Also orig_argv[argc] is already NULL as it was copied from argv
(which always ends with NULL).
But if it makes the analyzer happy I'm fine with adding one of
them in the code, with an additional comment saying it was
added to overcome an analyzer false-positive.
Thanks,
Uri.
Uri.
+ orig_argv[argc] = NULL;
context = g_option_context_new(NULL);
g_option_context_add_main_entries(context, entries, NULL);
I would say better to disable Clang test instead. The code is perfectly
fine. argc is at least 1 (the executable name!) and argv is always terminated
with NULL (that's the standard!).
See https://clang-analyzer.llvm.org/faq.html.
I don't know where this -1 come, but EXIT_FAILURE (which is usually 1) is the standard
return for main function.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel