C++: Add more random sources in cpp/uncontrolled-arithmetic#6154
Conversation
…eems to be a common source of false positives on LGTM.
|
CPP Language Tests are failing due to changes in the internal repo. I've created an internal PR to fix it. That one should be merged with this one. |
geoffw0
left a comment
There was a problem hiding this comment.
LGTM.
I wondered if it might be worth recognizing boost::random, the stuff in <random>, or common Mersenne Twister implementations (that were popular before all this stuff was standardized)? I'm leaning towards "no" however, because they're all more complex cases than rand and friends, and I'm not sure the payoff would be worth it.
| this.getNumberOfParameters() = 1 | ||
| } | ||
|
|
||
| override FunctionOutput getFunctionOutput() { result.isParameterDeref(0) } |
There was a problem hiding this comment.
The model of rand_s is nice and simple. 👍
| exists(int n | | ||
| source.asDefiningArgument() = call.getArgument(n) and | ||
| rand.getFunctionOutput().isParameterDeref(n) | ||
| ) |
There was a problem hiding this comment.
It's a shame we don't have a helper to convert Call + FunctionOutput-> Expr (to be clear, I'm not asking for one right now).
There was a problem hiding this comment.
Yeah, I agree. It would definitely be a good thing to have this. I guess we include such a predicate in semmle.code.cpp.models.interfaces.FunctionInputsAndOutputs.qll at some point.
This PR adds a couple more sources of randomness to
cpp/uncontrolled-arithmetic.I thought this would be a super trivial thing to do, but then I found Microsoft's rand_s function. That one behaves pretty much like
rand(enough so that we actually want to include it in the query), but with the crucial difference that it returns its result by updating a pointer that it's been passed. This makes it difficult to model using the oldTaintTrackingConfigurationlibrary.So as a bonus this PR also updates the query to use a
TaintTracking::Configuration🎉 .Now that we have more sources in this query, I also noticed a couple new false positives on LGTM. See for instance the result on
chakra-core/ChakraCorehere. This PR fixes these false positives by recognizing all bitwise operations as barriers.The idea is that, if a source of randomness is used in a bitwise operation, it's probably because the expression is used as a bit pattern, and thus overflow isn't a concern.
(Part of https://github.com/github/codeql-c-team/issues/553.)