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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

My 'fix' is just because we really have an extra char* which
seems to be uninitialized.

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

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]