1:22 p.m.
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.
nbdkit-1.30.7/filters/multi-conn/multi-conn.c:46: included_from: Included from here.
nbdkit-1.30.7/common/utils/cleanup.h:56:3: note: in expansion of macro
'ACQUIRE_LOCK_FOR_CURRENT_SCOPE_1'
nbdkit-1.30.7/filters/multi-conn/multi-conn.c:208:3: note: in expansion of macro
'ACQUIRE_LOCK_FOR_CURRENT_SCOPE'
nbdkit-1.30.7/filters/multi-conn/multi-conn.c:46: included_from: Included from here.
nbdkit-1.30.7/common/utils/vector.h:132:12: note: in definition of macro
'DEFINE_VECTOR_TYPE'
nbdkit-1.30.7/common/utils/vector.h:132:12: note: in definition of macro
'DEFINE_VECTOR_TYPE'
nbdkit-1.30.7/common/utils/vector.h:116:3: note: in definition of macro
'DEFINE_VECTOR_TYPE'
nbdkit-1.30.7/common/utils/vector.h:44: included_from: Included from here.
nbdkit-1.30.7/filters/multi-conn/multi-conn.c:46: included_from: Included from here.
nbdkit-1.30.7/filters/multi-conn/multi-conn.c:87:1: note: in expansion of macro
'DEFINE_VECTOR_TYPE'
nbdkit-1.30.7/filters/multi-conn/multi-conn.c:46: included_from: Included from here.
nbdkit-1.30.7/common/utils/vector.h:132:12: note: in definition of macro
'DEFINE_VECTOR_TYPE'
nbdkit-1.30.7/filters/multi-conn/multi-conn.c:45: included_from: Included from here.
nbdkit-1.30.7/common/utils/cleanup.h:58:41: note: in definition of macro
'ACQUIRE_LOCK_FOR_CURRENT_SCOPE_1'
nbdkit-1.30.7/common/include/unique-name.h:40:37: note: in expansion of macro
'XXUNIQUE_NAME'
nbdkit-1.30.7/common/include/unique-name.h:41:34: note: in expansion of macro
'XUNIQUE_NAME'
nbdkit-1.30.7/common/utils/cleanup.h:56:45: note: in expansion of macro
'NBDKIT_UNIQUE_NAME'
nbdkit-1.30.7/filters/multi-conn/multi-conn.c:208:3: note: in expansion of macro
'ACQUIRE_LOCK_FOR_CURRENT_SCOPE'
# 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'
# 225| }
# 226| if (group_vector_append (&groups, g) == -1)
# 227|-> return -1;
# 228| g->name = h->name;
# 229| h->name = NULL;
**********************************************************************
*** 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;
**********************************************************************
*** 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
|
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
4:13 p.m.
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
4:43 p.m.
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
4:47 p.m.
On Wed, Jul 13, 2022 at 04:43:38PM -0500, Eric Blake wrote:
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.
Thanks!
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
898
days inactive
899
days old
3 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones