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
