Called C++ object pointer is null in network/nethost.cc

this is a false positive from what I can say. the c++ object pointer that is marked as being a 0 pointer will always have a value, as this part of the code is only reached if delta time is bigger than 0, but this is only the case, if a game is running and thus d->game is != 0

I just rechecked and indeed checkHungClients() is only called in situations where a game is running. It is either called after a if (d->game) check or through a NET_CMD_TIME, NET_CMD_PLAYERCOMMAND or NET_CMD_SYNCREPORT package from a client. All three are only submitted in game.

Of course there might be some way to create a modified Widelands version to send one of those packages and getting it pass the different checks to finally end up in checkHungClients, but actually I do not see one at the moment.

of course we can ensilent clang if we put an if (!d->game) return; statement at the beginning of checkHungClients(), but I still see no relevant reason for it ;)

What do you think Hans Joachim? Maybe I miss something here... :)

Skimming through some of these reports it seem like most of them are false positives (e.g. c++ object being called is NULL seems to be very frequent but most unlikely in most scenarios). I recommend trying to add assert statements graciously around the bug reports - according to the web site, a lot of code paths are then pruned by default and should no longer be recorded. And if the assert is really wrong in any case we will know immediately.

Hm, ok. To make a long story short, I saw llvm 3.2 was released recently and decided to take the latest trunk for a run in their static code analysis tool to see if it found anything new. And indeed it found a lot more issues than the previous run I had did.

Based on the dead assignments/divide by zero issues and what I remembered from last time, that part of the results seemed free of false positives. I have to admit though, that c++ is not the language I'm most proficient in, so a lot of these were more or less forwarded for others to take a look at. The thought about false positives did occur to me when I saw dereferenced null pointers had increased from one or two to more than 70 though, which is one of the reasons why I didn't do anything with them.

I apologize if these reports got rather noisy, as the goal was to identify issues in the code which can be discovered by using the right tools. The next time I see a sharp increase in some type of error, I'll post one or two instead to see whether it is considered a real issue or not. Again, I'm sorry and I hope I didn't ruin the new year for you already. :)

Well, no :)!

Of course I may only speak for myself, but for me I think there is nothing you could or should say sorry for. I wasn't sure it isn't a real issue until I rechecked it - and I am searching through the code at the moment anyways ;) ... So the new year still is a positive one until now ;)

By the way: Happy new year to you, Hans Joachim!

My coding mind is warped enough to get excited at seeing these issues!

A bug report marked invalid is better than a bug report never filed :-)

I also agree with Peter and Mark. It is very valuable that you invest time in running static code analysis over widelands and it did find quite some issues with the code. And also here all the issues are relevant because clang cannot know about the invariances that we have established in Widelands.

I am more concerned how to shut clang up here - I read that assert() statement prunes search paths so that is why I was proposing them as a possible solution.

Take a closer look at that report. As far as I understand it, it outlines a scenario in which the Clang analyzer believes that the null pointer access could be triggered. The numbers in the annotation indicate the order in which control flows through the code.

Look at around line 2925: the scenario presented actually _starts_ by taking a branch in which d->game is known to be non-null, and later on it is claimed that d->game is null.

Add the fact that, apparently, the scenario runs through the loop in checkHungClients() four times before noticing the potential bug, and this starts looking like a real bug in the Clang analyzer rather than "just" a false positive.

Add the assert, because that certainly doesn't hurt, and if you feel particularly adventurous file a report to the Clang people.

fixed in bzr rev. 6534 with an assert

Eeeexcept that clang still claims it is present. I have to admit, I don't see how it could be null after explicitly asserting that it isn't. Starting to sound very much like a false positive to me as well.

Hm, I wonder if we should just leave these bug reports be for now, and then I'll rerun clang-analyzer once llvm 3.3 is out to see what it has to say then..

I've now had the time to run through the wl source code with scan-build from llvm 3.3 and the results are as following:
"Called C++ object pointer is null" dropped from 11 to 2 issues
"Dereference of null pointer" dropped from 77 to 1 issue
The other numbers show only minor changes. While the code base has changed a lot in the mean time, I think we can safely say these were false positives and move on. I'll clean up the other bug reports opened related to this. :)

Btw, the new report found some memory-related issues. They are only six in total, but not wanting to repeat this situation, I'm going to to things slightly differently. I'll file one bug for each category initially, and if they are valid, I can file the rest afterwards. Ok?

As always: thanks for taking the time doing this. The static analyzer has proven to be very useful, the true positives are only a small downside to this.

I commented on all the new bugs I saw - let me know if I missed one.

Aah, I forgot to mention: how can I run this myself on the code? A small tutorial would be nice.

How to run scan-build on the code (along with explanation/caveats):

cmake $WL_SOURCE \
 -DCMAKE_BUILD_TYPE=Debug \
 -DWL_PORTABLE=true \
 -DCMAKE_C_COMPILER=/usr/lib/clang-analyzer/scan-build/ccc-analyzer \
 -DCMAKE_CXX_COMPILER=/usr/lib/clang-analyzer/scan-build/c++-analyzer \
&& scan-build make

Basically just run `scan-build make` instead of `make`.

Note that in order for it to check anything you need to override the compiler variables to use c**-analyzer like I've done when calling cmake above. FIrst of all, this is sort of mentioned in the documentation, but they don't give any examples so I didn't get it to work properly until after a colleague explained it to me. Secondly, the location of the analyzers might differ from platform to platform, the example above is where they are located on Arch. (Looks like it is in /usr/share/clang/scan-build/c++-analyzer on Debian-based platforms.)

After you've successfully run `scan-build make` it will tell you how many issues it found, which directory it stored the report it and how to view the report in your favorite browser (`scan-view directory`).

Protip: if it runs through the compilation, but at the end tells you it didn't find any issues and deletes the directory containing the report, it's likely misconfigured somehow. At least in my experience. ^^

Let me know if you run into any trouble.

See also http://clang-analyzer.llvm.org/scan-build.html for more info

Thanks for this howto. It is more involved that I expected it to be - so I am grateful if you keep the bug tracker posted with the current state of affairs :).

I've now made a script for running scan-build which can be found utils/scan-build.sh. Simply run it from the root of the repo to get a report. Currently only works on Arch and Debian-based platforms, so patches welcome :)

>Currently only works on Arch and Debian-based platforms, so patches welcome :)
The script has since then been simplified quite a bit and should now run on all platforms (capable of running shells scripts). Turns out it was enough to add `scan-build` in front of the cmake command for it to set the necessary compiler options.

Released in build-18 rc1.