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
Ian Campbell
2013-Nov-20 14:02 UTC
Re: Coverity issues with block stubdom support in libxl
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.