[Spice-devel] [linux/vd_agent v1 1/2] covscan: check and initialize argv's copy
Uri Lublin
uril at redhat.com
Tue Aug 27 14:12:56 UTC 2019
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 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));
>>>> > # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> 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 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.
>
> 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 at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>
>>
More information about the Spice-devel
mailing list