Hi Ilya,

On Thu, Aug 08, 2019 at 12:45:33PM +0500, ???? ??????? wrote:
> Hello,
> 
> coverity found tens of "null pointer dereference".
> also, there's a good correlation, after "now fixed, good catch" coverity
> usually dismiss some bug.
> 
> should we revisit those findings ?

Not necessarily. The real problem with these reports is that they're
focused on inconsistency in the code resulting from repeated patterns
or copy-paste of some blocks, but coverity cannot guess what should
be *documented* as the valid call conditions. The most common pattern
resulting in this is the following :

   function foo(pointer p) {
        (...)
        if (p && p->field1) {
             do_something(p->field1);
        }
        (...)
        if (p && p->field2) {
             do_something_else(p->field2);
        }
        (...)
   }

Then later the API changes a bit so that foo() cannot be called with
a NULL pointer, or automatically allocates it, thus such code gets
inserted at the top :

        if (!p) {
             p = pool_alloc(pool);
             p->field = 1;
             p->field2 = 2;
        }

And this starts to trigger the suspicious tests at the bottom after
derefencing the field. Similarly if the decision to call foo() with
a possibly NULL pointer changes so that it never happens anymore, p
gets dereferenced early and the bottom code isn't updated to remove
these older tests, which confuses coverity.

Now it could be argued that changing the call convention of the
function should result in all tests being updated, but it could
also be argued that revisiting hundreds of unrelated lines of
code for a small change significantly increases the risk to
introduce new bugs by accident and doesn't bring any value at
all.

That's how you can end up with so many reports.

What I don't like with coverity reports is that they focus on check
inconsistencies in local code parts without knowing the global context,
and as such they require a lot of head scratching from a human to
figure whether they are right or wrong.

What I would suggest as a good practice when you find such a report,
is that you first try to figure from the code and description of the
called function if the issue may really happen or if it's a false
positive. If it can really happen based on the description, it's OK
to create a report. And similarly if there is no way to guess from
the function's description whether the situation can happen or not,
it means anyone using this function may fall into the trap and trigger
this latent bug, so I'm fine with an issue report as well to clarify
the situation. In other cases, these reports will cost you some work,
and induce some work on other developers as well to try to qualify
whether these are bugs or just noise, which is exactly the reason I
wanted to stay away from coverity reports in the first place.

Thanks,
willy

Reply via email to