search

github / codeql

CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security
https://codeql.github.com
MIT License
7.41k stars 1.48k forks source link

LGTM.com - false positive - Dereferenced variable may be null #2630

Open chrjohn opened 4 years ago

chrjohn commented 4 years ago

https://lgtm.com/projects/g/quickfix-j/quickfixj/snapshot/9657839bf46838645b670486e57852de40d34f04/files/quickfixj-core/src/main/java/quickfix/Message.java?sort=name&dir=ASC&mode=heatmap#xaced2743c6d7c82:1

To be honest it is quite tricky to find out that the variable group cannot be null. This is conveyed via boolean firstFieldFound which is only set when group has been assigned. See here:

https://lgtm.com/projects/g/quickfix-j/quickfixj/snapshot/9657839bf46838645b670486e57852de40d34f04/files/quickfixj-core/src/main/java/quickfix/Message.java?sort=name&dir=ASC&mode=heatmap#L707

But if firstFieldFound is false an Exception would be thrown from here: https://lgtm.com/projects/g/quickfix-j/quickfixj/snapshot/9657839bf46838645b670486e57852de40d34f04/files/quickfixj-core/src/main/java/quickfix/Message.java?sort=name&dir=ASC&mode=heatmap#L720

However, I wanted to make the null-check easier to spot for LGTM by removing the boolean variable and simply check for group!=null in checkFirstFieldFound() but the alert is still not removed. This can be seen here:

https://lgtm.com/projects/g/quickfix-j/quickfixj/rev/pr-e6092b79d8f5ba3d89e1644bd0153b8c5b22f5bb

The only alert not fixed by that PR is the possible null pointer dereference: https://lgtm.com/projects/g/quickfix-j/quickfixj/snapshot/9657839bf46838645b670486e57852de40d34f04/files/quickfixj-core/src/main/java/quickfix/Message.java?sort=name&dir=ASC&mode=heatmap#xaced2743c6d7c82:1

aschackmull commented 4 years ago

The nullness analysis is currently mostly intra-procedural, so if the check on firstFieldFound was inlined then it would likely work. Note that the analysis is able to rule out group being null a few lines above - likely due to an inferred correlation with previousOffset being -1.

chrjohn commented 4 years ago

I didn't want to inline the check on firstFieldFound because it is used in two different places.

Note that the analysis is able to rule out group being null a few lines above

You are right. E.g. Netbeans is not as clever and is also claiming a possible NP derefence there.

aschackmull commented 4 years ago

In any case, thank you for your FP report. I think we can reasonably extend the analysis to cover both the original code and your proposed fixup - they each require different extensions to the analysis as the inferences required are somewhat different. However fixing these is not at the top of the priority list at the moment so it may take me some time to get to.

aschackmull commented 4 years ago

For now, I've made a PR (https://github.com/Semmle/ql/pull/2640) to document the two FPs as unit tests.

chrjohn commented 4 years ago

No problem, thanks for your reply.