Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++: Improve function pointer resolution #17788

Merged
merged 6 commits into from
Oct 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* The function call target resolution algorithm has been improved to resolve more calls through function pointers. As a result, dataflow queries may have more results.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: feature
---
* Added a new predicate `DataFlow::getARuntimeTarget` for getting a function that may be invoked by a `Call` expression. Unlike `Call.getTarget` this new predicate may also resolve function pointers.
Original file line number Diff line number Diff line change
Expand Up @@ -1328,7 +1328,10 @@ predicate lambdaCreation(Node creation, LambdaCallKind kind, DataFlowCallable c)

/** Holds if `call` is a lambda call of kind `kind` where `receiver` is the lambda expression. */
predicate lambdaCall(DataFlowCall call, LambdaCallKind kind, Node receiver) {
call.(SummaryCall).getReceiver() = receiver.(FlowSummaryNode).getSummaryNode() and
(
call.(SummaryCall).getReceiver() = receiver.(FlowSummaryNode).getSummaryNode() or
call.asCallInstruction().getCallTargetOperand() = receiver.asOperand()
) and
exists(kind)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ private import SsaInternals as Ssa
private import DataFlowImplCommon as DataFlowImplCommon
private import codeql.util.Unit
private import Node0ToString
private import DataFlowDispatch as DataFlowDispatch
import ExprNodes

/**
Expand Down Expand Up @@ -2497,3 +2498,16 @@ class AdditionalCallTarget extends Unit {
*/
abstract Declaration viableTarget(Call call);
}

/**
* Gets a function that may be called by `call`.
*
* Note that `call` may be a call to a function pointer expression.
*/
Function getARuntimeTarget(Call call) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm always slightly confused. Is DataFlowUtil the file whose contents users may still depend upon, although it's located in internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct:

I don't know why this was done like this. This file structure is also present in the old AST dataflow, and we just copied it for IR dataflow.

We could actually change this, and maybe rename DataFlowUtil to DataFlowPublic to clear this confusion like it's been done for newer languages:

Screenshot 2024-10-17 at 11 01 15

(although it looks like every language still stores the file in internal for some reason 🤔)

exists(DataFlowCall dfCall | dfCall.asCallInstruction().getUnconvertedResultExpression() = call |
result = DataFlowDispatch::viableCallable(dfCall).asSourceCallable()
or
result = DataFlowImplCommon::viableCallableLambda(dfCall, _).asSourceCallable()
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ uniqueEnclosingCallable
| test.cpp:864:47:864:54 | call to source | Node should have one enclosing callable but has 0. |
| test.cpp:872:46:872:51 | call to source | Node should have one enclosing callable but has 0. |
| test.cpp:872:53:872:56 | 1 | Node should have one enclosing callable but has 0. |
| test.cpp:1126:33:1129:1 | {...} | Node should have one enclosing callable but has 0. |
| test.cpp:1127:3:1127:13 | reads_input | Node should have one enclosing callable but has 0. |
| test.cpp:1128:3:1128:21 | not_does_read_input | Node should have one enclosing callable but has 0. |
uniqueCallEnclosingCallable
| test.cpp:864:47:864:54 | call to source | Call should have one enclosing callable but has 0. |
| test.cpp:872:46:872:51 | call to source | Call should have one enclosing callable but has 0. |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ irFlow
| test.cpp:1069:9:1069:14 | call to source | test.cpp:1074:10:1074:10 | i |
| test.cpp:1069:9:1069:14 | call to source | test.cpp:1081:10:1081:10 | i |
| test.cpp:1117:27:1117:34 | call to source | test.cpp:1117:27:1117:34 | call to source |
| test.cpp:1132:11:1132:16 | call to source | test.cpp:1121:8:1121:8 | x |
| true_upon_entry.cpp:9:11:9:16 | call to source | true_upon_entry.cpp:13:8:13:8 | x |
| true_upon_entry.cpp:17:11:17:16 | call to source | true_upon_entry.cpp:21:8:21:8 | x |
| true_upon_entry.cpp:27:9:27:14 | call to source | true_upon_entry.cpp:29:8:29:8 | x |
Expand Down
16 changes: 16 additions & 0 deletions cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1115,4 +1115,20 @@ void indirect_sink_const_ref(const T&);

void test_temp_with_conversion_from_materialization() {
indirect_sink_const_ref(source()); // $ ir MISSING: ast
}

void reads_input(int x) {
sink(x); // $ ir MISSING: ast
}

void not_does_read_input(int x);

void (*dispatch_table[])(int) = {
reads_input,
not_does_read_input
};

void test_dispatch_table(int i) {
int x = source();
dispatch_table[i](x);
}
Loading