[Spice-devel] [linux/vd_agent v1 1/2] covscan: check and initialize argv's copy

[Uri Lublin]

On 8/27/19 1:27 PM, Frediano Ziglio wrote:
>>
>> From: Victor Toso <me at victortoso.com>
>>
>> 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));
>>   > #                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   > 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 at redhat.com>
>> ---
>>   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.

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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>