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

[Victor Toso]

Hi,

On Tue, Aug 27, 2019 at 06:27:27AM -0400, 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));
> > +    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.

Actually, the g_return_val_if_fail() is not needed. I was being
extra careful to eliminate the warning. Just initialized
orig_argv[argc] = NULL; seems enough as it might consider that
argc is zero and we would pass garbage on the execvp() with
orig_argv[0] instead of NULL.

Sending a v2.

> I don't know where this -1 come, but EXIT_FAILURE (which is
> usually 1) is the standard return for main function.
> 
> Frediano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190827/8953fc05/attachment-0001.sig>