Re: Coverity scan findings

2019-09-11 Thread GARDAIS Ionel


it depends on how haproxy is built (number of flags) 



BQ_BEGIN

we use most of available options when testing on coverity 
[ https://github.com/haproxy/haproxy/blob/master/.travis.yml#L8 | 
https://github.com/haproxy/haproxy/blob/master/.travis.yml#L8 ] 
can you share build command ? we may also set up sonar in travis-ci schedules. 
(personally, I find sonar too much noisy, but I agree, it finds bugs sometimes) 

BQ_END

I'm using 
$ make -j4 TARGET=linux-glibc USE_LIBCRYPT=1 USE_OPENSSL=1 USE_ZLIB=1 USE_NS= 

The shortest command from the travis file is 
$ make -j4 TARGET=linux-glibc USE_ZLIB=1 USE_PCRE=1 USE_OPENSSL=1 USE_WURFL=1 
WURFL_INC=contrib/wurfl WURFL_LIB=contrib/wurfl USE_DEVICEATLAS=1 
DEVICEATLAS_SRC=contrib/deviceatlas USE_NS= 

I'm using CentOS 6 to build. 


As Willy says, it generates lots of false-positive because static analysis of 
pointer-work is hard, especially in C. 
Most of C smart moves are interpreted as wrong behavior. 

--
232 avenue Napoleon BONAPARTE 92500 RUEIL MALMAISON
Capital EUR 219 300,00 - RCS Nanterre B 408 832 301 - TVA FR 09 408 832 301

Re: Coverity scan findings

2019-09-11 Thread Willy Tarreau
On Wed, Sep 11, 2019 at 10:08:46AM +0200, GARDAIS Ionel wrote:
> Please note that Sonarqube is scanning haproxy code too.
> Results are available at https://sonarcloud.io/dashboard?id=haproxy

Ah indeed.

> Some results are false positive but some are worth looking at.

Well, I've already lost one hour reading the first 7 ones. The problem
with such reports is that they are 99% false positives but are reported
out of context, requiring a lot of thinking to figure why they are wrong.
At least it would waste less human time for all those who analyse them
to join efforts on the same reports.

In addition it's worth noting that many of these reports are wrong by
*design* and that even if they are categorized as false positives, new
instances will regularly occur as code is being written because the
engine has huge difficulties understanding how pointers work. Every time
you dereference a "char area[0]" it reports an out-of-bounds array because
it cannot know that the whole object is larger and allocated on purpose.
Every time you subtract an offset from a pointer it's considered out of
bounds.

Overall that's why I don't like much these reports, I believe that the
manpower needed to address the numerous false positives is much higher
than what would be needed to spot and fix the real bugs. And moreover
it's quite demotivating for anyone to constantly have to analyse such
reports then figure why they are wrong, and having to explain to a robot
why one's code does in fact work.

Actually if some people want to help with bugs, what I can suggest is
to look at issues reported in github, look at backtraces resulting
from core dumps or functions that are cited in several bugs, consider
them as suspicious enough to warrant a deeper analysis, then start to
read them, figure where and how they are called, and figure if corner
cases are not properly covered. Such tools *might* help for this task
but you have your own judgement, and you'll progressively learn the
code and this will result in writing your own fixes. It will be more
fun than reading a bot's absurd reports.

In addition there is another great tool for spotting bugs, it's
Coccinelle. I already managed to find some in haproxy with it,
for example instances of "if (likely(expression) < 0)" or
"if (!strcmp(a, b) == 0)" or "if (!ptr == NULL)" which are all
human mistakes but end up being real bugs. Coccinelle managed
to find thousands of bugs in Linux and could very well find
many in haproxy. Just like we have with regtests, we could have
a collection of scripts for various cases that developers can
run from time to time on their code.

For example the script I'm using for "likely" looks like this:

  @rule1@
  identifier cond =~ "^\(likely\|unlikely\)$";
  expression E;
  statement S;
  binary operator b = {<,>,<=,>=};
  @@
  - if (cond(E) b 0)
  + if (cond(E b 0))
  S

I run it on all files with "spatch" so that it patches the
suspicious locations. This then allows me to use "git diff" to
inspect what it found, possibly "git commit" to confirm and fix
them, or "git checkout" to abort. Needless to say I'm not good
at writing such scripts so I'm not using them often, and when
I want to catch an occurrence of "!...NULL", I just use "git
grep" which is faster to sort out than to remember how to write
the script above. But for complex expressions and constructions
it's really worth investing some time on it.

Cheers,
Willy



Re: Coverity scan findings

2019-09-11 Thread Илья Шипицин
it depends on how haproxy is built (number of flags)

we use most of available options when testing on coverity

https://github.com/haproxy/haproxy/blob/master/.travis.yml#L8

can you share build command ? we may also set up sonar in travis-ci
schedules.

(personally, I find sonar too much noisy, but I agree, it finds bugs
sometimes)

ср, 11 сент. 2019 г. в 13:08, GARDAIS Ionel <
ionel.gard...@tech-advantage.com>:

>
> > On Tue, Sep 10, 2019 at 08:29:38PM +0500,  ??? wrote:
> > > those findings are mostly mess (maybe, except few real bugs).
> > > I do not mind sharing those findings with community, Willy ?
> > > we need more manpower here.
> >
> > Oh no problem! I'm not the one asking to hide bugs, the more eyeballs
> > on bug reports, the faster these ones will be sorted out! Also if one
> > fears that this could help a black hat guy find a vulnerability and
> > exploit it, mind you that these people already spend time scanning the
> > same code (with and without tools) and spot bugs in advance without
> > relying on our public reports anyway.
>
>
> Please note that Sonarqube is scanning haproxy code too.
> Results are available at https://sonarcloud.io/dashboard?id=haproxy
>
> Some results are false positive but some are worth looking at.
> --
> 232 avenue Napoleon BONAPARTE 92500 RUEIL MALMAISON
> Capital EUR 219 300,00 - RCS Nanterre B 408 832 301 - TVA FR 09 408 832 301
>
>


Re: Coverity scan findings

2019-09-11 Thread GARDAIS Ionel


> On Tue, Sep 10, 2019 at 08:29:38PM +0500,  ??? wrote:
> > those findings are mostly mess (maybe, except few real bugs).
> > I do not mind sharing those findings with community, Willy ?
> > we need more manpower here.
> 
> Oh no problem! I'm not the one asking to hide bugs, the more eyeballs
> on bug reports, the faster these ones will be sorted out! Also if one
> fears that this could help a black hat guy find a vulnerability and
> exploit it, mind you that these people already spend time scanning the
> same code (with and without tools) and spot bugs in advance without
> relying on our public reports anyway.


Please note that Sonarqube is scanning haproxy code too.
Results are available at https://sonarcloud.io/dashboard?id=haproxy

Some results are false positive but some are worth looking at.
--
232 avenue Napoleon BONAPARTE 92500 RUEIL MALMAISON
Capital EUR 219 300,00 - RCS Nanterre B 408 832 301 - TVA FR 09 408 832 301




Re: Coverity scan findings

2019-09-11 Thread Willy Tarreau
On Tue, Sep 10, 2019 at 08:29:38PM +0500,  ??? wrote:
> those findings are mostly mess (maybe, except few real bugs).
> I do not mind sharing those findings with community, Willy ?
> we need more manpower here.

Oh no problem! I'm not the one asking to hide bugs, the more eyeballs
on bug reports, the faster these ones will be sorted out! Also if one
fears that this could help a black hat guy find a vulnerability and
exploit it, mind you that these people already spend time scanning the
same code (with and without tools) and spot bugs in advance without
relying on our public reports anyway.

The only thing I want to avoid is to have to spend my time rejecting
patches that fix false positives, because it would end up with coverity
slowing down our progress and causing diminishing quality instead of
the opposite.

Cheers,
Willy



Re: Coverity scan findings

2019-09-11 Thread Willy Tarreau
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



Re: Coverity scan findings

2019-09-10 Thread Dinko Korunic
Hi Dave,

Just browse to https://scan.coverity.com/projects/haproxy 
 and make a request for access, 
I’ll gladly add you to the project.

> On 10 Sep 2019, at 16:49, Dave Chiluk  wrote:
> 
> Are these scans publicly available *(I'm looking for a link)?  They look 
> interesting, but without line numbers it looks a lot less useful.
> 
> Dave.
> 
> On Thu, Aug 8, 2019 at 2:49 AM Илья Шипицин  > 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 ?
> 
> 
> 


Kind regards,
D.

-- 
Dinko Korunic   ** Standard disclaimer applies **
Sent from OSF1 osf1v4b V4.0 564 alpha