Coverity issues with block stubdom support in libxl

Coverity issues with block stubdom support in libxl

[Andrew Cooper]

Hello,

Coverity has found some issues.

CID1130521 - USE_AFTER_FREE
libxl.c: device_complete().  Conditional free of aodev->dev followed by
unconditional use in the log message.  Reeording these operations would
be the sensible fix.

CID 1130517 and 1130518 - RESOURCE_LEAK
libxl_dm.c: libxl__spawn_qdisk_backend().  If libxl_spawn_spawn()
returns an error, the logfile_w and null file handles are leaked.

There was also a query about whether libxl__exec takes ownership of the
handles, which I shall defer to those who know libxl better than I.


The following are not strictly your bugs, as you only copied the code. 
However, they need fixing.

CID 1130516 - NEGATIVE_RETURNS
xl_cmdimpl.c: do_daemonize().  Coverity complains that it is possible to
dup2(logfile, 1); with logfile as -1.  It appears that the uses of
CHK_ERRNO() are bogus.  CHK_ERRNO() itself tests for "(call) < 0".  In
do_daemonize(), the tests therefore become "if ( ((call) < 0) < 0 )"
which fails to catch the error condition.  I suspect removing the
trailing "<0" in the CHK_ERRNO() calls should fix the error (but I would
appreciate a second opinion on this).

I believe that the above is also the cause of CID 1130519 -
RESOURCE_LEAK leaking nullfd.

CID 1130520 - UNREACHABLE
xl_cmdimpl.c: do_daemonize().  The "for (;;)" is bogus.  The body of the
loop unconditionally exits on the first iteration.

~Andrew

Re: Coverity issues with block stubdom support in libxl

[Ian Campbell]

On Wed, 2013-11-20 at 13:49 +0000, Andrew Cooper wrote:
> CID 1130516 - NEGATIVE_RETURNS
> xl_cmdimpl.c: do_daemonize().  Coverity complains that it is possible to
> dup2(logfile, 1); with logfile as -1.  It appears that the uses of
> CHK_ERRNO() are bogus.  CHK_ERRNO() itself tests for "(call) < 0".  In
> do_daemonize(), the tests therefore become "if ( ((call) < 0) < 0 )"
> which fails to catch the error condition.  I suspect removing the
> trailing "<0" in the CHK_ERRNO() calls should fix the error (but I would
> appreciate a second opinion on this).

Yes, I think that macro is a bit dodgy -- it's called in two different
contexts and handles neither of them properly AFAICT!

See http://marc.info/?l=xen-devel&m=138018557503300 for previous
discussion.