C++: IR query for redundant null check #894
Conversation
rdmarsh2
left a comment
rdmarsh2
left a comment
There was a problem hiding this comment.
I think we shouldn't add this to a suite unless we're confident we want it in production in 1.20.
The value numbering suggestion isn't urgent, especially if this is only intended to make sure the IR is being tested in the nightly builds.
| void test_indirect(int **p) { | ||
| int x; | ||
| x = **p; | ||
| if (*p == nullptr) { // BAD [NOT DETECTED] |
There was a problem hiding this comment.
Does using value numbering on sourceValue in the query clause make the query detect this?
There was a problem hiding this comment.
I thought it would, and I tried, but it didn't work. If you find out how to make it work, please let me know. I'd like to improve this query later so it generates more results.
cpp/config/suites/c/correctness
Outdated
| + semmlecode-cpp-queries/Likely Bugs/Arithmetic/BadAdditionOverflowCheck.ql: /Correctness/Common Errors | ||
| + semmlecode-cpp-queries/Likely Bugs/NestedLoopSameVar.ql: /Correctness/Common Errors | ||
| + semmlecode-cpp-queries/Likely Bugs/UseInOwnInitializer.ql: /Correctness/Common Errors | ||
| + semmlecode-cpp-queries/Likely Bugs/RedundantNullCheckSimple.ql: /Correctness/Common Errors |
There was a problem hiding this comment.
I'd prefer not to add this to a suite if we aren't ready to run it in production. I'm not sure if anybody is using the correctness suite, but I know of a 1.19 deployment where security suites are in active use.
There was a problem hiding this comment.
Point taken. I've rebased so we're no longer adding to correctness but instead creating a new suite named experimental.
jbj
force-pushed
the
ir-RedundantNullCheckSimple
branch
from
16422b9 to
9e0f3a2
Compare
| // before the deref. That's still not okay because when they're in the same | ||
| // basic block then the deref is unavoidable even if the check concluded that | ||
| // the pointer was null. To follow this idea to its full generality, we | ||
| // should also give an alert when `check` post-dominates `deref`. |
There was a problem hiding this comment.
Can we have test coverage for these two cases then?
I'm happy as long as the performance of this query is thought to be at least comparable to the worst from those suites (where is it at the moment?). The standards for noisiness are lower than what we aim for on LGTM so that's unlikely to be a huge concern. It should have qhelp. |
This new query is not written because it's the most interesting query we could write but because it's an IR-based query whose results are easy to verify.
This suite isn't referenced from anywhere yet, but it'll be included in a standard ODASA dist because the dist includes all files in the `c` and `cpp` directories. We can modify the nightly test jobs to include the experimental suite.
jbj
force-pushed
the
ir-RedundantNullCheckSimple
branch
from
636f21f to
9f2fdbb
Compare
|
Rebased to fix semantic merge conflict. |
Suites are not only used in legacy products. Customers who use the command line tools can, and do, use suites with |
|
@lukecartey I've taken the query out of the default suites after I wrote the PR description. They're now in a new suite named |
|
@geoffw0 Do you find this good to merge? This PR has no effect on its own, but my next step will be to add the new |
|
@geoffw0 ? |
|
I think it's good enough to merge, yes (sorry for the delay noticing this again). As a query it looks solid but narrow (I only found one result - in MongoDB - but I think it's a good result) and lacks qhelp (which I'd like to see written for this soon). But I understand the goal is to get an IR based query on a nightly Jenkins job, which seems like a good goal, so I'm happy to merge this now. |
4 participants
This new query is not written because it's the most interesting query we could write but because it's an IR-based query whose results are easy to verify. Maybe we can evolve it into catching more bugs at some point.
This query is not ready for LGTM, but it can go in the suites used by the nightly query comparison jobs. Is it too risky to add it to
cpp/config/suites/{c,cpp}/correctness? Those suites should only be in use by legacy products, but maybe there still exist deployments that pick up their queries from these files. Do you know, @geoffw0?