11:04 a.m.
On 07/01/2015 10:20 AM, Peter Krempa wrote:
On Wed, Jul 01, 2015 at 10:03:44 -0400, John Ferlan wrote:
> The following Coverity issues are flagged in Coverity 7.6.1 compared
> to not being seen in 7.5.1 - this series just addresses those issues
> before 7.6.1 is installed in our upstream Jenkins environment:
>
> http://jenkins-virt.usersys.redhat.com/job/libvirt-coverity/
This is a Red Hat internal link where other users don't have access to
this definitely should not be published to a mailing list at least to
avoid frustration to non-redhatters.
hmm.. for some reason I thought it was an external reference - I know
we've been running many tests thru Jenkins on an external CentOS server,
so my brain said this was too... But now you've reminded me that the
Coverity ones had to be internal because of the license... In any case -
it wasn't intentional... Sorry about that.
>
> It seems the bulk are essentially false positives, but we can do something
> in order to work around them.
So why are we doing anything about that? Coverity is the software that
should be fixed here.
While coverity is a very useful tool in some cases where it reports
actual errors the noise it generates is sometimes quite unbearable and
as it looks the new version added just a few false positives, but no
actual fault.
Additionally if we continue to patch up the mistakes rather than
reporting it we might as well as end up by flagging something with the
sa_assert() macro that will change into an actual error in later
patches and then we won't be able to detect that.
As of such I think that libvirt should mostly fix just actual errors
found by coverity and people who run coverity on the libvirt code base
should rather report the errorrs to the coverity vendor to fix the false
positive notifications rather than working that around by silencing it.
</rant>
Peter
Certainly understand your frustration with Coverity's false positive;
however, remember perhaps one person's "bug" is someone else's
"feature". The more complex the code and the deeper the calling stack,
the more difficult it is for artificial intelligence tools to model
every possible combination. There's a point in which even they have to
let the human intelligence (sic) take over.
A particular pain point are API's where a status or count is returned as
well as returning a pointer to something. We know from reading the man
pages or reading the code that the only way that pointer can be filled
in is if the status/count is a particular value (usually non negative -
so zero or more). However, that's more difficult for modeling code
which tries "all" paths when it finds a condition and then flags when
there's a "chance" for something to go wrong. For example, code such as
the following causes these types of issues:
char *retparam = NULL;
if ((rc = function(param, param, &retparam)) < 0)
goto error;
if (retparam->field)
Coverity will scan the code and "flag" that retparam could be a problem
here. It will further dive into 'function' and determine code paths from
there that could perhaps return >= 0 such that retparam wouldn't be
filled in. If 'function' is a macro for some other function which, then
calls yet another function, which is a macro for something else (ad
nauseum), it becomes more difficult to model, thus Coverity "flags" it
as a potential problem for a human to resolve/check. False positives are
a way of life - they get worse for stricter checking - we only do the
'basic' checks.
In the case of the 'virpci' changes (patches 1-3) - the "eventual" call
to virVasprintfInternal() can fail returning -1 and setting '*strp =
NULL;" - perfectly reasonable... The caller virAsprintfInternal doesn't
check ret, but passes it back to it's caller via macro in virstring.h:
# define virAsprintf(strp, ...) \
virAsprintfInternal(
Eventually virPCIFile (for example) returns -1 or 0 based on that
return, but it seems being able to correlate that return back in any of
the callers cannot happen. Coverity wants to mock both the true and
false path of the condition, thus the error. Whether it's macros or
variable call stacks that confuses things, I'm not quite sure. But, I'll
assume there was some corner case for someone else's code that caused
the modeling code to not flag it when someone felt it should. Who really
knows, I don't follow Coverity development that closely.
Sometimes, issues are even more opaque...
Consider the case from openvz.conf.c changes:
if (VIR_STRDUP(str, value) < 0)
goto error;
iden = strtok_r(line, " ", &saveptr);
Seems perfectly fine, right? However, if one digs into
VIR_STRDUP()/virStrdup() they find this:
*dest = NULL;
if (!src)
return 0;
So if by chance 'value' was NULL (we don't prevent or check it), then
str will be NULL and that's perfectly reasonable. The caller is
expected to check it, right? But in this case in order to get into the
function we have 'assumed' the 'value' is non-NULL based on the positive
return:
ret = openvzReadVPSConfigParam(veid, param, &temp);
if (ret > 0) {
if (openvzParseBarrierLimit(temp, &barrier, &limit)) {
So now Coverity cannot model the called functions, rather it has to
model the calling functions and "know" that ret > 0 means temp != NULL.
Since we can do this ourselves by reading the header of the ConfigParam
function, but the artificial intelligence just doesn't see that since it
doesn't know to correlate "ret" with "temp". Thus we just have to
teach
it and move on.
John