[Libguestfs] A few Coverity errors in nbdkit

[Eric Blake]

On Tue, Jul 12, 2022 at 07:22:31PM +0100, Richard W.M. Jones wrote:
> Hi Eric,
> 
> None of this is important, and may not even be bugs, but here are a
> few issues raised with the latest run of Coverity on nbdkit 1.30.7.
> 
> 
> **********************************************************************
> *** filters/multi-conn
> 
> It seems as if "group_vector" is really leaked from this filter.  I
> think it needs an unload handler to free it.
> 
> 
> Error: GCC_ANALYZER_WARNING (CWE-401): [#def75]
> nbdkit-1.30.7/filters/multi-conn/multi-conn.c:195:14: warning[-Wanalyzer-malloc-leak]: leak of 'g'

> nbdkit-1.30.7/common/utils/vector.h:44: included_from: Included from here.
> #  193|       r = next->can_multi_conn (next);
> #  194|       if (r == -1)
> #  195|->       return -1;
> #  196|       if (r == 0)
> #  197|         h->mode = EMULATE;
> 
> Error: CLANG_WARNING: [#def76]
> nbdkit-1.30.7/filters/multi-conn/multi-conn.c:227:15: warning[unix.Malloc]: Potential leak of memory pointed to by 'g'

Yep, seems like an easy fix to make.

> **********************************************************************
> *** server/protocol-handshake-newstyle.c
> 
> I think Coverity has a point here.  Certainly there is code before
> this point which assigns conn->top_context = NULL, and it's not
> immediately clear to me that the code that Coverity has highlighted is
> unreachable in that case.
> 
> 
> Error: CLANG_WARNING: [#def234]
> nbdkit-1.30.7/server/protocol-handshake-newstyle.c:657:34: warning[core.NonNullParamChecker]: Null pointer passed to 1st parameter expecting 'nonnull'
> #  655|             case NBD_INFO_DESCRIPTION:
> #  656|               {
> #  657|->               const char *desc = backend_export_description (conn->top_context);
> #  658|   
> #  659|                 if (!desc) {
> 
> Error: CLANG_WARNING: [#def235]
> nbdkit-1.30.7/server/protocol-handshake-newstyle.c:676:19: warning[core.NonNullParamChecker]: Null pointer passed to 1st parameter expecting 'nonnull'
> #  674|                 uint32_t minimum, preferred, maximum;
> #  675|   
> #  676|->               if (backend_block_size (conn->top_context,
> #  677|                                         &minimum, &preferred, &maximum) == -1)
> #  678|                   return -1;

I'll look into those.

> 
> 
> **********************************************************************
> *** server/public.c
> 
> Any clue about what this error means?
> 
> 
> Error: ASSERT_SIDE_EFFECT (CWE-1006): [#def239]
> nbdkit-1.30.7/server/public.c:728: assert_side_effect: Argument "quit" of assert() has a side effect because the variable is volatile.  The containing function might work differently in a non-debug build.
> #  726|      * event, we know the connection should be shutting down.
> #  727|      */
> #  728|->   assert (quit ||
> #  729|             (conn && conn->nworkers > 0 && connection_get_status () < 1) ||
> #  730|             (conn && (fds[2].revents & (POLLRDHUP | POLLHUP | POLLERR |

Interesting.  Because 'quit' is volatile, Coverity is arguing that the
mere act of reading it may trigger a side effect; where an NDEBUG
build skips the read.  A (maybe non-obvious) fix would be:

bool has_quit = quit;
assert (has_quit || ... );

so that the contents of the assert() no longer read a volatile variable.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org