Skip to content
Open
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
33 changes: 21 additions & 12 deletions cpp/ql/lib/semmle/code/cpp/dataflow/internal/FlowSummaryImpl.qll
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ module Input implements InputSig<Location, DataFlowImplSpecific::CppDataFlow> {

class SummarizedCallableBase = Function;

class SourceBase = Void;
class SourceBase extends Void {
Location getLocation() { none() }
}

class SinkBase = Void;
class SinkBase = SourceBase;

class FlowSummaryCallBase = CallInstruction;

Expand Down Expand Up @@ -132,10 +134,22 @@ module Input implements InputSig<Location, DataFlowImplSpecific::CppDataFlow> {

private import Make<Location, DataFlowImplSpecific::CppDataFlow, Input> as Impl

private module StepsInput implements Impl::Private::StepsInputSig {
Impl::Private::SummaryNode getSummaryNode(Node n) {
result = n.(FlowSummaryNode).getSummaryNode()
private module Input2 implements Impl::Private::InputSig2 {
private import codeql.util.Void

class SourceSinkReportingElement extends Void {
Location getLocation() { none() }

DataFlowCallable getEnclosingCallable() { none() }

SourceSinkReportingElement getASuccessor(Impl::Private::SummaryComponent sc) { none() }
Comment on lines +140 to +145

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Having one class to model both sources and sink elements makes it impossible to give each kind their own toString, right?

For C/C++ I'm hoping that we can keep the source output argument output from toString for the source node, but keep the arg output for toString when it's a sink. For example, I really hope that we could keep the source node's toString as source output argument, but have the sink node's toString give arg in:

void test() {
  source(arg);
  sink(arg);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You can override the default toString implementations provided by SourceOutputNode and SinkInputNode, where sources and sinks are distinguished. I'm curious, however, why is source output argument a better string representation than simply arg (which I believe all other languages use)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I personally like it because the two nodes represent different things (i.e., the value before or after a call), and I like to have those things to have different string representations 🤷

On a less subjective note: It's super useful for debugging customer confusion about dataflow. Customers often confuse node.asDefiningArgument() (which selects the post-update node of an argument), and node.asExpr() - probably since they both give you the argument Expr. To make the confusion even worse, if you have code such as:

int* x = ...;
source(x);
sink(x);

then using source.asExpr() and sink.asExpr() will work and give you the expected flow. However, since asExpr won't give you the post-update node that solution won't work in cases such as:

struct S { int x; };
S s;
source(s.x);
sink(s.x);

So if a customer reports a flow path starting at an argument node and the node is not of the form ... output argument then I know they've done something wrong, and they will be missing flow.

It's not the end of the world to not have this, but it's a useful feature that I'd rather avoid losing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In Rust, source(x) and source(s.x) should both use the post-update nodes of x and s.x, resp., so they will have the [post] ... prefix in the textual representation to distinguish them from the values before the call. This logic exists for MaD defined sources here in the getSourceDataFlowNode predicate.

@MathiasVP MathiasVP Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh that part I agree with. What I'm saying is that, in C++, if a customer writes a query like:

module Config implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) { ... source.asExpr() ... }
}

then this will give them the right flow for source(x), but not for source(s.x). I don't see how this also won't apply to Rust?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that also applies to Rust, and perhaps all languages should have an asDefiningArgument predicate for this. But I still think the textual representation using [post] ... should be enough for debugging.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indeed, I would also be totally okay with [post] ... for the outgoing argument. As long as the toString is different from the toString of the expression node I'm happy 🙂

}
}

private import Impl::Private::Make2<Input2> as Impl2

private module StepsInput implements Impl2::StepsInputSig {
Impl2::SummaryNode getSummaryNode(Node n) { result = n.(FlowSummaryNode).getSummaryNode() }

DataFlowCall getACall(Public::SummarizedCallable sc) {
result.getStaticCallTarget().getUnderlyingCallable() = sc
Expand All @@ -148,12 +162,6 @@ private module StepsInput implements Impl::Private::StepsInputSig {
pragma[only_bind_out](out.getIndirectionIndex())
)
}

DataFlowCallable getSourceNodeEnclosingCallable(Input::SourceBase source) { none() }

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponentStack s) { none() }

Node getSinkNode(Input::SinkBase sink, Impl::Private::SummaryComponent sc) { none() }
}

module SourceSinkInterpretationInput implements
Expand Down Expand Up @@ -270,8 +278,9 @@ module SourceSinkInterpretationInput implements

module Private {
import Impl::Private
import Impl2

module Steps = Impl::Private::Steps<StepsInput>;
module Steps = Impl2::Steps<StepsInput>;

module External {
import Impl::Private::External
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1535,10 +1535,10 @@ class FlowSummaryNode extends Node, TFlowSummaryNode {
}

override DataFlowCallable getEnclosingCallable() {
result = FlowSummaryImpl::Private::getEnclosingCallable(this.getSummaryNode())
result = this.getSummaryNode().getEnclosingCallable()
}

override Location getLocationImpl() { result = this.getSummarizedCallable().getLocation() }
override Location getLocationImpl() { result = this.getSummaryNode().getLocation() }

override string toStringImpl() { result = this.getSummaryNode().toString() }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,7 @@ private module Cached {
)
or
// models-as-data summarized flow
FlowSummaryImpl::Private::Steps::summaryJumpStep(n1.(FlowSummaryNode).getSummaryNode(),
n2.(FlowSummaryNode).getSummaryNode())
FlowSummaryImpl::Private::Steps::summaryJumpStep(n1, n2)
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,7 @@ private module Cached {
model = ""
or
// models-as-data summarized flow
FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom,
nodeTo.(FlowSummaryNode).getSummaryNode(), true, model)
FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, true, model)
}

private predicate simpleInstructionLocalFlowStep(Operand opFrom, Instruction iTo) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,7 @@ private module Cached {
model = ""
or
// models-as-data summarized flow
FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom,
nodeTo.(FlowSummaryNode).getSummaryNode(), false, model)
FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, false, model)
or
// object->field conflation for content that is a `TaintInheritingContent`.
exists(DataFlow::ContentSet f |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -714,8 +714,7 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo, string model) {
) and
model = ""
or
FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom,
nodeTo.(FlowSummaryNode).getSummaryNode(), true, model)
FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, true, model)
}

/**
Expand Down Expand Up @@ -1778,7 +1777,7 @@ class FlowSummaryNode extends NodeImpl, TFlowSummaryNode {
}

override DataFlowCallable getEnclosingCallableImpl() {
result.asSummarizedCallable() = this.getSummarizedCallable()
result = this.getSummaryNode().getEnclosingCallable()
}

override DataFlowType getDataFlowType() {
Expand All @@ -1789,7 +1788,7 @@ class FlowSummaryNode extends NodeImpl, TFlowSummaryNode {

override ControlFlowNode getControlFlowNodeImpl() { none() }

override Location getLocationImpl() { result = this.getSummarizedCallable().getLocation() }
override Location getLocationImpl() { result = this.getSummaryNode().getLocation() }

override string toStringImpl() { result = this.getSummaryNode().toString() }
}
Expand Down Expand Up @@ -2085,8 +2084,7 @@ predicate jumpStep(Node pred, Node succ) {
)
)
or
FlowSummaryImpl::Private::Steps::summaryJumpStep(pred.(FlowSummaryNode).getSummaryNode(),
succ.(FlowSummaryNode).getSummaryNode())
FlowSummaryImpl::Private::Steps::summaryJumpStep(pred, succ)
or
succ = pred.(LocalFunctionCreationNode).getAnAccess(false)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,17 @@ module Input implements InputSig<Location, DataFlowImplSpecific::CsharpDataFlow>
)
}

class SourceBase = Void;
class SourceBase extends Void {
Location getLocation() { none() }
}

class SinkBase = SourceBase;

class SinkBase = Void;
class FlowSummaryCallBase = SourceBase;

class FlowSummaryCallBase = Void;
DataFlowCallable getSummarizedCallableAsDataFlowCallable(SummarizedCallableBase c) {
result.asSummarizedCallable() = c
}

predicate neutralElement(SummarizedCallableBase c, string kind, string provenance, boolean isExact) {
interpretNeutral(c, kind, provenance, isExact)
Expand Down Expand Up @@ -122,7 +128,21 @@ module Input implements InputSig<Location, DataFlowImplSpecific::CsharpDataFlow>

private import Make<Location, DataFlowImplSpecific::CsharpDataFlow, Input> as Impl

private module TypesInput implements Impl::Private::TypesInputSig {
private module Input2 implements Impl::Private::InputSig2 {
private import codeql.util.Void

class SourceSinkReportingElement extends Void {
Location getLocation() { none() }

DataFlowCallable getEnclosingCallable() { none() }

SourceSinkReportingElement getASuccessor(Impl::Private::SummaryComponent sc) { none() }
}
}

private import Impl::Private::Make2<Input2> as Impl2

private module TypesInput implements Impl2::TypesInputSig {
DataFlowType getSyntheticGlobalType(Impl::Private::SyntheticGlobal sg) {
exists(sg) and
result.asGvnType() = Gvn::getGlobalValueNumber(any(ObjectType t))
Expand Down Expand Up @@ -195,27 +215,15 @@ private module TypesInput implements Impl::Private::TypesInputSig {
)
}

DataFlowType getSourceType(Input::SourceBase source, Impl::Private::SummaryComponentStack s) {
none()
}

DataFlowType getSinkType(Input::SinkBase sink, Impl::Private::SummaryComponent sc) { none() }
DataFlowType getSourceSinkType(Input2::SourceSinkReportingElement e) { none() }
}

private module StepsInput implements Impl::Private::StepsInputSig {
Impl::Private::SummaryNode getSummaryNode(Node n) {
result = n.(FlowSummaryNode).getSummaryNode()
}
private module StepsInput implements Impl2::StepsInputSig {
Impl2::SummaryNode getSummaryNode(Node n) { result = n.(FlowSummaryNode).getSummaryNode() }

DataFlowCall getACall(Public::SummarizedCallable sc) {
sc = viableCallable(result).asSummarizedCallable()
}

DataFlowCallable getSourceNodeEnclosingCallable(Input::SourceBase source) { none() }

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponentStack s) { none() }

Node getSinkNode(Input::SinkBase sink, Impl::Private::SummaryComponent sc) { none() }
}

module SourceSinkInterpretationInput implements
Expand Down Expand Up @@ -339,9 +347,10 @@ module SourceSinkInterpretationInput implements

module Private {
import Impl::Private
import Impl::Private::Types<TypesInput>
import Impl2
import Types<TypesInput>

module Steps = Impl::Private::Steps<StepsInput>;
module Steps = Impl2::Steps<StepsInput>;

module External {
import Impl::Private::External
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,7 @@ private module Cached {
) and
model = ""
or
FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom,
nodeTo.(FlowSummaryNode).getSummaryNode(), false, model)
FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, false, model)
}
}

Expand Down
2 changes: 1 addition & 1 deletion go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ module Private {
result = this.getSummaryNode().getSummarizedCallable()
}

override Location getLocation() { result = this.getSummarizedCallable().getLocation() }
override Location getLocation() { result = this.getSummaryNode().getLocation() }

override string toString() { result = this.getSummaryNode().toString() }

Expand Down
3 changes: 1 addition & 2 deletions go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,7 @@ predicate jumpStep(Node n1, Node n2) {
n2 = recvRead
)
or
FlowSummaryImpl::Private::Steps::summaryJumpStep(n1.(FlowSummaryNode).getSummaryNode(),
n2.(FlowSummaryNode).getSummaryNode())
FlowSummaryImpl::Private::Steps::summaryJumpStep(n1, n2)
}

/**
Expand Down
3 changes: 1 addition & 2 deletions go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,7 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo, string model) {
any(FunctionModel m).flowStep(nodeFrom, nodeTo) and
model = "FunctionModel"
or
FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom,
nodeTo.(FlowSummaryNode).getSummaryNode(), true, model)
FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, true, model)
}

/**
Expand Down
39 changes: 26 additions & 13 deletions go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,20 @@ module Input implements InputSig<Location, DataFlowImplSpecific::GoDataFlow> {

class SummarizedCallableBase = Callable;

class SourceBase = Void;
class SourceBase extends Void {
Location getLocation() { none() }
}

class SinkBase = Void;
class SinkBase = SourceBase;

class FlowSummaryCallBase = Void;
class FlowSummaryCallBase = SourceBase;

predicate callableFromSource(SummarizedCallableBase c) { exists(c.getFuncDef()) }

DataFlowCallable getSummarizedCallableAsDataFlowCallable(SummarizedCallableBase c) {
result.asSummarizedCallable() = c
}

predicate neutralElement(
Input::SummarizedCallableBase c, string kind, string provenance, boolean isExact
) {
Expand Down Expand Up @@ -114,23 +120,29 @@ module Input implements InputSig<Location, DataFlowImplSpecific::GoDataFlow> {

private import Make<Location, DataFlowImplSpecific::GoDataFlow, Input> as Impl

private module StepsInput implements Impl::Private::StepsInputSig {
Impl::Private::SummaryNode getSummaryNode(Node n) {
result = n.(FlowSummaryNode).getSummaryNode()
private module Input2 implements Impl::Private::InputSig2 {
private import codeql.util.Void

class SourceSinkReportingElement extends Void {
Location getLocation() { none() }

DataFlowCallable getEnclosingCallable() { none() }

SourceSinkReportingElement getASuccessor(Impl::Private::SummaryComponent sc) { none() }
}
}

private import Impl::Private::Make2<Input2> as Impl2

private module StepsInput implements Impl2::StepsInputSig {
Impl2::SummaryNode getSummaryNode(Node n) { result = n.(FlowSummaryNode).getSummaryNode() }

DataFlowCall getACall(Public::SummarizedCallable sc) {
exists(DataFlow::CallNode call |
call.asExpr() = result and
call.getACalleeIncludingExternals() = sc
)
}

DataFlowCallable getSourceNodeEnclosingCallable(Input::SourceBase source) { none() }

Node getSourceNode(Input::SourceBase source, Impl::Private::SummaryComponentStack s) { none() }

Node getSinkNode(Input::SinkBase sink, Impl::Private::SummaryComponent sc) { none() }
}

module SourceSinkInterpretationInput implements
Expand Down Expand Up @@ -500,8 +512,9 @@ private predicate parseReturn(AccessPath::AccessPathTokenBase c, int n) {

module Private {
import Impl::Private
import Impl2

module Steps = Impl::Private::Steps<StepsInput>;
module Steps = Impl2::Steps<StepsInput>;

module External {
import Impl::Private::External
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,7 @@ private predicate localAdditionalForwardTaintStep(
or
any(AdditionalTaintStep a).step(pred, succ) and model = "AdditionalTaintStep"
or
FlowSummaryImpl::Private::Steps::summaryLocalStep(pred,
succ.(DataFlowPrivate::FlowSummaryNode).getSummaryNode(), false, model)
FlowSummaryImpl::Private::Steps::summaryLocalStep(pred, succ, false, model)
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ module Private {
result.asCallable() = n.(ImplicitInstanceAccess).getInstanceAccess().getEnclosingCallable() or
result.asCallable() = n.(MallocNode).getClassInstanceExpr().getEnclosingCallable() or
result = nodeGetEnclosingCallable(n.(ImplicitPostUpdateNode).getPreUpdateNode()) or
result.asSummarizedCallable() = n.(FlowSummaryNode).getSummarizedCallable() or
result = n.(FlowSummaryNode).getSummaryNode().getEnclosingCallable() or
result.asCallable() = n.(CaptureNode).getSynthesizedCaptureNode().getEnclosingCallable() or
result.asFieldScope() = n.(FieldValueNode).getField() or
result.asCallable() = any(Expr e | n.(AdditionalNode).nodeAt(e, _)).getEnclosingCallable() or
Expand Down Expand Up @@ -531,7 +531,7 @@ module Private {
result = this.getSummaryNode().getSummarizedCallable()
}

override Location getLocation() { result = this.getSummarizedCallable().getLocation() }
override Location getLocation() { result = this.getSummaryNode().getLocation() }

override string toString() { result = this.getSummaryNode().toString() }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,8 +199,7 @@ predicate jumpStep(Node node1, Node node2) {
any(AdditionalValueStep a).step(node1, node2) and
node1.getEnclosingCallable() != node2.getEnclosingCallable()
or
FlowSummaryImpl::Private::Steps::summaryJumpStep(node1.(FlowSummaryNode).getSummaryNode(),
node2.(FlowSummaryNode).getSummaryNode())
FlowSummaryImpl::Private::Steps::summaryJumpStep(node1, node2)
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,7 @@ private predicate simpleLocalFlowStep0(Node node1, Node node2, string model) {
or
cloneStep(node1, node2) and model = "CloneStep"
or
FlowSummaryImpl::Private::Steps::summaryLocalStep(node1, node2.(FlowSummaryNode).getSummaryNode(),
true, model)
FlowSummaryImpl::Private::Steps::summaryLocalStep(node1, node2, true, model)
}

/**
Expand Down
Loading
Loading