Libguestfs - Jul 2022 - A few Coverity errors in nbdkit

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

On Tue, Jul 12, 2022 at 04:13:28PM -0500, Eric Blake
wrote:
> 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.
> > 
> > **********************************************************************
> > *** 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.
> 
> I'll look into those.
The code setting conn->top_context = NULL is followed by continue (it
is only used as an error path); the NBD_INFO code (that uses
conn->top_context) is only reachable if finish_newstyle_options()
succeeded, which in turn means protocol_common_open() succeeded.  But
as chasing through different files is hard for Coverity (let alone
humans) to see that a successful protocol_common_open() implies
conn->top_context is non-NULL, I just added an assert().

I've pushed 18e1504d..21e43619 that should address all of these
Coverity findings.

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