Skip to content

Conversation

MathiasVP
Copy link
Contributor

Fixes https://github.com/github/codeql-c-analysis-team/issues/189.

It turns out that the set of functions which are ArrayFunctions with no output parameters is a strict superset of functions satisfying either PureStrFunction, StrLenFunction or PureMemFunction. And when you factor out the functions that satisfy either TaintFunction or DataFlowFunction (like we do in ExternalAPIDataNode), I think these two sets are equivalent.

@MathiasVP MathiasVP added the C++ label Nov 25, 2020
@MathiasVP MathiasVP requested review from a team as code owners November 25, 2020 17:11
@github-actions github-actions bot added the C# label Nov 25, 2020
…ee previosuly-used implementation models private.
@MathiasVP MathiasVP force-pushed the safe-external-api-function-use-model-interfaces-only branch from f165f49 to 7730f5d Compare November 25, 2020 17:21
@MathiasVP MathiasVP removed the C# label Nov 25, 2020
Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've discussed of whether "write side effects" includes writes to sockets, files, or memory not seen by the caller. I think we decided it does not, but for the purpose of DefaultSafeExternalAPIFunction I don't think we have to be precise about it.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

We've discussed of whether "write side effects" includes writes to sockets, files, or memory not seen by the caller.

I think that means we may want to refine the models in future. We also already have some special cases in Function.mayHaveSideEffects() that seem very similar to this.

@MathiasVP MathiasVP merged commit a4c060a into github:main Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants