C++: alias and side effect info for pure functions #1566
Conversation
| } | ||
|
|
||
| override predicate parameterEscapesOnlyViaReturn(int i) { | ||
| none() |
There was a problem hiding this comment.
The strstr family of functions return a pointer that's derived from (and may be equal to) their first parameter.
| or name = "strnlen" | ||
| or name = "strrchr" | ||
| or name = "strspn" | ||
| or name = "strstr" |
There was a problem hiding this comment.
By strstr family I meant also strcasestr, strchr, strchrnul, and strrchr. Instead of enumerating them, you can consider matching them by signature: it's all the functions on this list that return a pointer type.
jbj
left a comment
jbj
left a comment
There was a problem hiding this comment.
Sorry for torturing this PR so much. I've just been involved in fixing many hard-to-debug bugs that were caused by inaccurate modelling of standard-library functions. It looks like we'll be modelling many more of those functions in Q3, so I'd like the foundation to be robust.
| ) and | ||
| ( | ||
| output.isOutReturnValue() or | ||
| output.isOutReturnPointer() |
There was a problem hiding this comment.
I think the previous version of this was right:
... and
(
(
output.isOutReturnPointer() and
getUnspecifiedType() instanceof PointerType
) or
output.isOutReturnValue()
)
The new version claims that taint flows to the return pointer even if the return value is int.
|
|
||
| override predicate parameterEscapesOnlyViaReturn(int i) { | ||
| i = 0 and | ||
| getType().getUnspecifiedType() instanceof PointerType |
There was a problem hiding this comment.
Didn't we get rid of getType().getUnspecifiedType() chains?
| not ( | ||
| i = 0 and | ||
| getType().getUnspecifiedType() instanceof PointerType | ||
| ) |
There was a problem hiding this comment.
Can not ( ... ) be simplified to not parameterEscapesOnlyViaReturn(i)? That should make it more robust against future changes. Hard-coding i = 0 only once makes me only half as nervous about it.
| class PureFunction extends TaintFunction, SideEffectFunction { | ||
| PureFunction() { | ||
| exists(string name | | ||
| hasName(name) and |
There was a problem hiding this comment.
I know it's independent of this PR, but this use of hasName has bitten us several times, and I don't want it to bite us again. It's just very likely that class member functions have short names like abs. See, for example, #1023.
Please switch to hasGlobalName here and in PureStrFunction. If you want to make sure that std::abs is also matched, then it's time to implement the hasGlobalOrStdName predicate we've been talking about.
There was a problem hiding this comment.
I'll switch to hasGlobalName for now, and do hasGlobalOrStdName as a separate PR (since it should involve a pass through most of the library)
| override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { | ||
| exists (ParameterIndex i | | ||
| input.isInParameter(i) | ||
| ) and |
There was a problem hiding this comment.
If there's a function in the snapshot with 20 parameters, then this predicate claims that parameter 19 of abs flows to its return value. That seems wasteful and confusing.
I'd much prefer that we only define taint flow for parameters that actually exist. The same issue is in PureStrFunction.
To make this easier to detect in the future, we could make ParameterIndex private and put a bindingset annotation on isInParameter etc. But that's probably not for this PR.
There was a problem hiding this comment.
I've narrowed it to only report parameters that actually exist
| int PureFunctions(char *str1, char *str2, int x) { | ||
| int ret = strcmp(str1, str2); | ||
| ret += abs(x); | ||
| return ret; |
There was a problem hiding this comment.
You wrote in #1542 (comment) that this PR was about the side effects of strlen, so shouldn't that be included in the test?
2 participants
No description provided.